[PATCH net] packet: fix use after free race in send path when dev is released

[PATCH net] packet: fix use after free race in send path when dev is released

[Daniel Borkmann]

Salam reported a use after free bug in PF_PACKET that occurs when
we're sending out frames on a socket bound device and suddenly the
netdevice is being unregistered. It appears that commit 827d9780
introduced a possible race condition between {t,}packet_snd and
packet_notifier. In the case of a bound socket, packet_notifier can
drop the last reference to the net_device and {t,}packet_snd might
end up sending a packet over a freed net_device.

To avoid reverting 827d9780 entirely, we could make use of po->running
member that gets reset when we're calling __unregister_prot_hook() in
packet_notifier() when we receive NETDEV_DOWN or NETDEV_UNREGISTER
notification. Plus, we still need to hold ref to the netdev, so
that we can assure it won't be released while we're in send path.
We try to avoid holding the bind lock all over send code as this
would likely make it worse when multiple threads want to send on
the same socket (in mmap code we're holding the pg_vec_lock mutex
all over the send code; we should try to get rid of that at some
point in time!). This at least buys us the possibility that we don't
have to walk over the entire dev list in dev_get_by_index() that we
would need to do as before 827d9780, which can just hurt as well
with many devices registered.

Fixes: 827d978037d7 ("af-packet: Use existing netdev reference for bound sockets.")
Signed-off-by: Daniel Borkmann <dborkman@redhat.com>
Signed-off-by: Salam Noureddine <noureddine@aristanetworks.com>
Tested-by: Salam Noureddine <noureddine@aristanetworks.com>
Cc: Ben Greear <greearb@candelatech.com>
---
 net/packet/af_packet.c | 33 ++++++++++++++++++++-------------
 1 file changed, 20 insertions(+), 13 deletions(-)

diff --git a/net/packet/af_packet.c b/net/packet/af_packet.c
index 2e8286b..385b2cc 100644
--- a/net/packet/af_packet.c
+++ b/net/packet/af_packet.c
@@ -1045,6 +1045,19 @@ static void *__prb_previous_block(struct packet_sock *po,
 	return prb_lookup_block(po, rb, previous, status);
 }
 
+static struct net_device *dev_get_packet_sock(struct packet_sock *po)
+{
+	struct net_device *dev;
+
+	spin_lock(&po->bind_lock);
+	dev = po->prot_hook.dev;
+	if (dev)
+		dev_hold(dev);
+	spin_unlock(&po->bind_lock);
+
+	return dev;
+}
+
 static void *packet_previous_rx_frame(struct packet_sock *po,
 					     struct packet_ring_buffer *rb,
 					     int status)
@@ -2057,7 +2070,6 @@ static int tpacket_snd(struct packet_sock *po, struct msghdr *msg)
 	struct sk_buff *skb;
 	struct net_device *dev;
 	__be16 proto;
-	bool need_rls_dev = false;
 	int err, reserve = 0;
 	void *ph;
 	struct sockaddr_ll *saddr = (struct sockaddr_ll *)msg->msg_name;
@@ -2070,7 +2082,7 @@ static int tpacket_snd(struct packet_sock *po, struct msghdr *msg)
 	mutex_lock(&po->pg_vec_lock);
 
 	if (saddr == NULL) {
-		dev = po->prot_hook.dev;
+		dev	= dev_get_packet_sock(po);
 		proto	= po->num;
 		addr	= NULL;
 	} else {
@@ -2084,7 +2096,6 @@ static int tpacket_snd(struct packet_sock *po, struct msghdr *msg)
 		proto	= saddr->sll_protocol;
 		addr	= saddr->sll_addr;
 		dev = dev_get_by_index(sock_net(&po->sk), saddr->sll_ifindex);
-		need_rls_dev = true;
 	}
 
 	err = -ENXIO;
@@ -2094,7 +2105,7 @@ static int tpacket_snd(struct packet_sock *po, struct msghdr *msg)
 	reserve = dev->hard_header_len;
 
 	err = -ENETDOWN;
-	if (unlikely(!(dev->flags & IFF_UP)))
+	if (unlikely(!(dev->flags & IFF_UP) || !po->running))
 		goto out_put;
 
 	size_max = po->tx_ring.frame_size
@@ -2173,8 +2184,7 @@ out_status:
 	__packet_set_status(po, ph, status);
 	kfree_skb(skb);
 out_put:
-	if (need_rls_dev)
-		dev_put(dev);
+	dev_put(dev);
 out:
 	mutex_unlock(&po->pg_vec_lock);
 	return err;
@@ -2212,7 +2222,6 @@ static int packet_snd(struct socket *sock,
 	struct sk_buff *skb;
 	struct net_device *dev;
 	__be16 proto;
-	bool need_rls_dev = false;
 	unsigned char *addr;
 	int err, reserve = 0;
 	struct virtio_net_hdr vnet_hdr = { 0 };
@@ -2228,7 +2237,7 @@ static int packet_snd(struct socket *sock,
 	 */
 
 	if (saddr == NULL) {
-		dev = po->prot_hook.dev;
+		dev	= dev_get_packet_sock(po);
 		proto	= po->num;
 		addr	= NULL;
 	} else {
@@ -2240,7 +2249,6 @@ static int packet_snd(struct socket *sock,
 		proto	= saddr->sll_protocol;
 		addr	= saddr->sll_addr;
 		dev = dev_get_by_index(sock_net(sk), saddr->sll_ifindex);
-		need_rls_dev = true;
 	}
 
 	err = -ENXIO;
@@ -2250,7 +2258,7 @@ static int packet_snd(struct socket *sock,
 		reserve = dev->hard_header_len;
 
 	err = -ENETDOWN;
-	if (!(dev->flags & IFF_UP))
+	if (unlikely(!(dev->flags & IFF_UP) || !po->running))
 		goto out_unlock;
 
 	if (po->has_vnet_hdr) {
@@ -2386,15 +2394,14 @@ static int packet_snd(struct socket *sock,
 	if (err > 0 && (err = net_xmit_errno(err)) != 0)
 		goto out_unlock;
 
-	if (need_rls_dev)
-		dev_put(dev);
+	dev_put(dev);
 
 	return len;
 
 out_free:
 	kfree_skb(skb);
 out_unlock:
-	if (dev && need_rls_dev)
+	if (dev)
 		dev_put(dev);
 out:
 	return err;
-- 
1.8.3.1

Re: [PATCH net] packet: fix use after free race in send path when dev is released

[Eric Dumazet]

On Wed, 2013-11-20 at 00:08 +0100, Daniel Borkmann wrote:
> Salam reported a use after free bug in PF_PACKET that occurs when
> we're sending out frames on a socket bound device and suddenly the
> netdevice is being unregistered. It appears that commit 827d9780
> introduced a possible race condition between {t,}packet_snd and
> packet_notifier. In the case of a bound socket, packet_notifier can
> drop the last reference to the net_device and {t,}packet_snd might
> end up sending a packet over a freed net_device.
> 
> To avoid reverting 827d9780 entirely, we could make use of po->running
> member that gets reset when we're calling __unregister_prot_hook() in
> packet_notifier() when we receive NETDEV_DOWN or NETDEV_UNREGISTER
> notification. Plus, we still need to hold ref to the netdev, so
> that we can assure it won't be released while we're in send path.
> We try to avoid holding the bind lock all over send code as this
> would likely make it worse when multiple threads want to send on
> the same socket (in mmap code we're holding the pg_vec_lock mutex
> all over the send code; we should try to get rid of that at some
> point in time!). This at least buys us the possibility that we don't
> have to walk over the entire dev list in dev_get_by_index() that we
> would need to do as before 827d9780, which can just hurt as well
> with many devices registered.
> 
> Fixes: 827d978037d7 ("af-packet: Use existing netdev reference for bound sockets.")
> Signed-off-by: Daniel Borkmann <dborkman@redhat.com>
> Signed-off-by: Salam Noureddine <noureddine@aristanetworks.com>
> Tested-by: Salam Noureddine <noureddine@aristanetworks.com>
> Cc: Ben Greear <greearb@candelatech.com>
> ---
>  net/packet/af_packet.c | 33 ++++++++++++++++++++-------------
>  1 file changed, 20 insertions(+), 13 deletions(-)
> 
> diff --git a/net/packet/af_packet.c b/net/packet/af_packet.c
> index 2e8286b..385b2cc 100644
> --- a/net/packet/af_packet.c
> +++ b/net/packet/af_packet.c
> @@ -1045,6 +1045,19 @@ static void *__prb_previous_block(struct packet_sock *po,
>  	return prb_lookup_block(po, rb, previous, status);
>  }
>  
> +static struct net_device *dev_get_packet_sock(struct packet_sock *po)
> +{
> +	struct net_device *dev;
> +
> +	spin_lock(&po->bind_lock);
> +	dev = po->prot_hook.dev;
> +	if (dev)
> +		dev_hold(dev);
> +	spin_unlock(&po->bind_lock);
> +
> +	return dev;
> +}
> +

...

dev_get_by_index() would be faster in fact.

Are you sure rcu_read_lock() is not possible ?

rcu_read_lock();
dev = rcu_dereference(po->active_device);
if (dev)
	dev_hold(dev);
rcu_read_unlock();

Re: [PATCH net] packet: fix use after free race in send path when dev is released

[David Miller]

From: Daniel Borkmann <dborkman@redhat.com>
Date: Wed, 20 Nov 2013 00:08:23 +0100

> To avoid reverting 827d9780 entirely, we could make use of po->running
> member that gets reset when we're calling __unregister_prot_hook() in
> packet_notifier() when we receive NETDEV_DOWN or NETDEV_UNREGISTER
> notification. Plus, we still need to hold ref to the netdev, so
> that we can assure it won't be released while we're in send path.

The avoidance of the atomic ref counting of the network device is the
main performance gain we get from that commit.

Now we'll be doing the refcount _and_ taking a spinlock, it'll be
worse than beforehand.

And this is doubly silly because we already have a reference
when we install the device into po->prot_hook.dev

I bet you can fix this by just deferring the NETDEV_UNREGISTER
AF_PACKET notifier work to RCU.

Re: [PATCH net] packet: fix use after free race in send path when dev is released

[Daniel Borkmann]

On 11/20/2013 02:34 AM, David Miller wrote:
> From: Daniel Borkmann <dborkman@redhat.com>
> Date: Wed, 20 Nov 2013 00:08:23 +0100
>
>> To avoid reverting 827d9780 entirely, we could make use of po->running
>> member that gets reset when we're calling __unregister_prot_hook() in
>> packet_notifier() when we receive NETDEV_DOWN or NETDEV_UNREGISTER
>> notification. Plus, we still need to hold ref to the netdev, so
>> that we can assure it won't be released while we're in send path.
>
> The avoidance of the atomic ref counting of the network device is the
> main performance gain we get from that commit.
>
> Now we'll be doing the refcount _and_ taking a spinlock, it'll be
> worse than beforehand.
>
> And this is doubly silly because we already have a reference
> when we install the device into po->prot_hook.dev
>
> I bet you can fix this by just deferring the NETDEV_UNREGISTER
> AF_PACKET notifier work to RCU.

Yep, will try if this approach works, in other words doing the earlier
exit via !po->running, plus deferring the dev_put() et al to RCU.

Re: [PATCH net] packet: fix use after free race in send path when dev is released

[David Miller]

From: Daniel Borkmann <dborkman@redhat.com>
Date: Wed, 20 Nov 2013 09:30:17 +0100

> On 11/20/2013 02:34 AM, David Miller wrote:
>> From: Daniel Borkmann <dborkman@redhat.com>
>> Date: Wed, 20 Nov 2013 00:08:23 +0100
>>
>>> To avoid reverting 827d9780 entirely, we could make use of po->running
>>> member that gets reset when we're calling __unregister_prot_hook() in
>>> packet_notifier() when we receive NETDEV_DOWN or NETDEV_UNREGISTER
>>> notification. Plus, we still need to hold ref to the netdev, so
>>> that we can assure it won't be released while we're in send path.
>>
>> The avoidance of the atomic ref counting of the network device is the
>> main performance gain we get from that commit.
>>
>> Now we'll be doing the refcount _and_ taking a spinlock, it'll be
>> worse than beforehand.
>>
>> And this is doubly silly because we already have a reference
>> when we install the device into po->prot_hook.dev
>>
>> I bet you can fix this by just deferring the NETDEV_UNREGISTER
>> AF_PACKET notifier work to RCU.
> 
> Yep, will try if this approach works, in other words doing the earlier
> exit via !po->running, plus deferring the dev_put() et al to RCU.

Thank you.  You might have to wrap the sendmsg path in an rcu lock
sequence.

Re: [PATCH net] packet: fix use after free race in send path when dev is released

[Eric Dumazet]

On Wed, 2013-11-20 at 15:07 -0500, David Miller wrote:
> From: Daniel Borkmann <dborkman@redhat.com>

> > Yep, will try if this approach works, in other words doing the earlier
> > exit via !po->running, plus deferring the dev_put() et al to RCU.
> 
> Thank you.  You might have to wrap the sendmsg path in an rcu lock
> sequence.

Note that dev_put() / dev_hold() are blazingly fast ;)

Re: [PATCH net] packet: fix use after free race in send path when dev is released

[David Miller]

From: Eric Dumazet <eric.dumazet@gmail.com>
Date: Wed, 20 Nov 2013 12:15:59 -0800

> On Wed, 2013-11-20 at 15:07 -0500, David Miller wrote:
>> From: Daniel Borkmann <dborkman@redhat.com>
> 
>> > Yep, will try if this approach works, in other words doing the earlier
>> > exit via !po->running, plus deferring the dev_put() et al to RCU.
>> 
>> Thank you.  You might have to wrap the sendmsg path in an rcu lock
>> sequence.
> 
> Note that dev_put() / dev_hold() are blazingly fast ;)

Oh yes, per-cpu counters, almost forgot :-)))