From 260916dfb48c374f7840f3b86e69afd3afdb6e96 Mon Sep 17 00:00:00 2001 From: Herbert Xu Date: Wed, 1 Jun 2016 11:43:00 +0800 Subject: [PATCH 1/2] macvlan: Fix potential use-after free for broadcasts When we postpone a broadcast packet we save the source port in the skb if it is local. However, the source port can disappear before we get a chance to process the packet. This patch fixes this by holding a ref count on the netdev. It also delays the skb->cb modification until after we allocate the new skb as you should not modify shared skbs. Fixes: 412ca1550cbe ("macvlan: Move broadcasts into a work queue") Signed-off-by: Herbert Xu Signed-off-by: David S. Miller --- drivers/net/macvlan.c | 10 ++++++++-- 1 file changed, 8 insertions(+), 2 deletions(-) diff --git a/drivers/net/macvlan.c b/drivers/net/macvlan.c index cb01023eab41..a71fa592b1fb 100644 --- a/drivers/net/macvlan.c +++ b/drivers/net/macvlan.c @@ -305,11 +305,14 @@ static void macvlan_process_broadcast(struct work_struct *w) rcu_read_unlock(); + if (src) + dev_put(src->dev); kfree_skb(skb); } } static void macvlan_broadcast_enqueue(struct macvlan_port *port, + const struct macvlan_dev *src, struct sk_buff *skb) { struct sk_buff *nskb; @@ -319,8 +322,12 @@ static void macvlan_broadcast_enqueue(struct macvlan_port *port, if (!nskb) goto err; + MACVLAN_SKB_CB(nskb)->src = src; + spin_lock(&port->bc_queue.lock); if (skb_queue_len(&port->bc_queue) < MACVLAN_BC_QUEUE_LEN) { + if (src) + dev_hold(src->dev); __skb_queue_tail(&port->bc_queue, nskb); err = 0; } @@ -429,8 +436,7 @@ static rx_handler_result_t macvlan_handle_frame(struct sk_buff **pskb) goto out; } - MACVLAN_SKB_CB(skb)->src = src; - macvlan_broadcast_enqueue(port, skb); + macvlan_broadcast_enqueue(port, src, skb); return RX_HANDLER_PASS; } From 9c127a016e66a85edaad6f9a674d0d1dce93d251 Mon Sep 17 00:00:00 2001 From: Herbert Xu Date: Wed, 1 Jun 2016 11:45:44 +0800 Subject: [PATCH 2/2] macvlan: Avoid unnecessary multicast cloning Currently we always queue a multicast packet for further processing, even if none of the macvlan devices are subscribed to the address. This patch optimises this by adding a global multicast filter for a macvlan_port. Note that this patch doesn't handle the broadcast addresses of the individual macvlan devices correctly, if they are not all identical to vlan->lowerdev. However, this is already broken because there is no mechanism in place to update the individual multicast filters when you change the broadcast address. If someone cares enough they should fix this by collecting all broadcast addresses for a macvlan as we do for multicast and unicast. Signed-off-by: Herbert Xu Signed-off-by: David S. Miller --- drivers/net/macvlan.c | 40 ++++++++++++++++++++++++++++++++++------ 1 file changed, 34 insertions(+), 6 deletions(-) diff --git a/drivers/net/macvlan.c b/drivers/net/macvlan.c index a71fa592b1fb..0c65bd914aed 100644 --- a/drivers/net/macvlan.c +++ b/drivers/net/macvlan.c @@ -49,6 +49,7 @@ struct macvlan_port { bool passthru; int count; struct hlist_head vlan_source_hash[MACVLAN_HASH_SIZE]; + DECLARE_BITMAP(mc_filter, MACVLAN_MC_FILTER_SZ); }; struct macvlan_source_entry { @@ -419,6 +420,8 @@ static rx_handler_result_t macvlan_handle_frame(struct sk_buff **pskb) port = macvlan_port_get_rcu(skb->dev); if (is_multicast_ether_addr(eth->h_dest)) { + unsigned int hash; + skb = ip_check_defrag(dev_net(skb->dev), skb, IP_DEFRAG_MACVLAN); if (!skb) return RX_HANDLER_CONSUMED; @@ -436,7 +439,9 @@ static rx_handler_result_t macvlan_handle_frame(struct sk_buff **pskb) goto out; } - macvlan_broadcast_enqueue(port, src, skb); + hash = mc_hash(NULL, eth->h_dest); + if (test_bit(hash, port->mc_filter)) + macvlan_broadcast_enqueue(port, src, skb); return RX_HANDLER_PASS; } @@ -722,12 +727,12 @@ static void macvlan_change_rx_flags(struct net_device *dev, int change) } } -static void macvlan_set_mac_lists(struct net_device *dev) +static void macvlan_compute_filter(unsigned long *mc_filter, + struct net_device *dev, + struct macvlan_dev *vlan) { - struct macvlan_dev *vlan = netdev_priv(dev); - if (dev->flags & (IFF_PROMISC | IFF_ALLMULTI)) { - bitmap_fill(vlan->mc_filter, MACVLAN_MC_FILTER_SZ); + bitmap_fill(mc_filter, MACVLAN_MC_FILTER_SZ); } else { struct netdev_hw_addr *ha; DECLARE_BITMAP(filter, MACVLAN_MC_FILTER_SZ); @@ -739,10 +744,33 @@ static void macvlan_set_mac_lists(struct net_device *dev) __set_bit(mc_hash(vlan, dev->broadcast), filter); - bitmap_copy(vlan->mc_filter, filter, MACVLAN_MC_FILTER_SZ); + bitmap_copy(mc_filter, filter, MACVLAN_MC_FILTER_SZ); } +} + +static void macvlan_set_mac_lists(struct net_device *dev) +{ + struct macvlan_dev *vlan = netdev_priv(dev); + + macvlan_compute_filter(vlan->mc_filter, dev, vlan); + dev_uc_sync(vlan->lowerdev, dev); dev_mc_sync(vlan->lowerdev, dev); + + /* This is slightly inaccurate as we're including the subscription + * list of vlan->lowerdev too. + * + * Bug alert: This only works if everyone has the same broadcast + * address as lowerdev. As soon as someone changes theirs this + * will break. + * + * However, this is already broken as when you change your broadcast + * address we don't get called. + * + * The solution is to maintain a list of broadcast addresses like + * we do for uc/mc, if you care. + */ + macvlan_compute_filter(vlan->port->mc_filter, vlan->lowerdev, NULL); } static int macvlan_change_mtu(struct net_device *dev, int new_mtu)