packet: hold bind lock when rebinding to fanout hook
Subsystems (3)
PATCH 1/1 packet: hold bind lock when rebinding to fanout hook
Packet socket bind operations must hold the po->bind_lock. This keeps po->running consistent with whether the socket is actually on a ptype list to receive packets.
fanout_add unbinds a socket and its packet_rcv/tpacket_rcv call, then binds the fanout object to receive through packet_rcv_fanout.
Make it hold the po->bind_lock when testing po->running and rebinding. Else, it can race with other rebind operations, such as that in packet_set_ring from packet_rcv to tpacket_rcv. Concurrent updates can result in a socket being added to a fanout group twice, causing use-after-free KASAN bug reports, among others.
Reported independently by both trinity and syzkaller. Verified that the syzkaller reproducer passes after this patch.
--- net/packet/af_packet.c | 16 +++++++++++----- 1 file changed, 11 insertions(+), 5 deletions(-)
net/packet/af_packet.c +11 -5
@@ -1684,10 +1684,6 @@ static int fanout_add(struct sock *sk, u16 id, u16 type_flags)mutex_lock (&fanout_mutex); - err = -EINVAL ; -if (!po->running) - goto out; - err = -EALREADY ;if (po->fanout) goto out;
@@ -1749,7 +1745,10 @@ static int fanout_add(struct sock *sk, u16 id, u16 type_flags)list_add (&match->list, &fanout_list); } err = -EINVAL ; -if (match->type == type && + +spin_lock (&po->bind_lock); +if (po->running && + match->type == type && match->prot_hook.type == po->prot_hook.type && match->prot_hook.dev == po->prot_hook.dev) { err = -ENOSPC ;
@@ -1761,6 +1760,13 @@ static int fanout_add(struct sock *sk, u16 id, u16 type_flags) err =0 ; } } +spin_unlock (&po->bind_lock); + +if (err && !refcount_read (&match->sk_ref)) { +list_del (&match->list); +kfree (match); + } + out:if (err && rollover) {kfree (rollover);
Signature · 1 line
2.14.1.690.gbb1197296e-goog
Quoted 50 lines · Willem de Bruijn
Packet socket bind operations must hold the
po->bind_lock. This keepspo->runningconsistent with whether the socket is actually on a ptype list to receive packets.
fanout_addunbinds a socket and its packet_rcv/tpacket_rcv call, then binds the fanout object to receive throughpacket_rcv_fanout.Make it hold the
po->bind_lockwhen testingpo->runningand rebinding. Else, it can race with other rebind operations, such as that inpacket_set_ringfrompacket_rcvtotpacket_rcv. Concurrent updates can result in a socket being added to a fanout group twice, causing use-after-free KASAN bug reports, among others.Reported independently by both trinity and syzkaller. Verified that the syzkaller reproducer passes after this patch.
Reported-by nixioaming <[email protected]>Signed-off-by Willem de Bruijn <[email protected]>--- net/packet/af_packet.c | 16 +++++++++++----- 1 file changed, 11 insertions(+), 5 deletions(-)net/packet/af_packet.c +4
-5@@ -1684,10 +1684,6 @@ static int fanout_add(struct sock *sk, u16 id, u16 type_flags)mutex_lock (&fanout_mutex); - err = -EINVAL ; -if (!po->running) - goto out; - err = -EALREADY ;if (po->fanout) goto out;@@ -1749,7 +1745,10 @@ static int fanout_add(struct sock *sk, u16 id, u16 type_flags)list_add (&match->list, &fanout_list); } err = -EINVAL ; -if (match->type == type && + +spin_lock (&po->bind_lock); +if (po->running && + match->type == type && match->prot_hook.type == po->prot_hook.type && match->prot_hook.dev == po->prot_hook.dev) { err = -ENOSPC ;
net/packet/af_packet.c static int fanout_add(struct sock *sk, u16 id, u16 type_flags) · line 1765
Quoted 6 lines
net/packet/af_packet.c +3
-0@@ -1761,6 +1760,13 @@ static int fanout_add(struct sock *sk, u16 id, u16 type_flags) err =0 ; } } +spin_unlock (&po->bind_lock); + +if (err && !refcount_read (&match->sk_ref)) {
It seems sk_ref is always read/changed undermutex_lock(&fanout_mutex) protection.
Not sure why we use a refcount_t (or an atomic_t in older kernels)
All these atomic/spinlock/mutexes are a maze.
Quoted 7 lines
net/packet/af_packet.c +4
-0+list_del (&match->list); +kfree (match); + } + out:if (err && rollover) {kfree (rollover);
Willem de Bruijn
maintainerQuoted 7 lines · Willem de Bruijn
Packet socket bind operations must hold the
po->bind_lock. This keepspo->runningconsistent with whether the socket is actually on a ptype list to receive packets.
fanout_addunbinds a socket and its packet_rcv/tpacket_rcv call, then binds the fanout object to receive throughpacket_rcv_fanout.
Quoted 9 lines
Make it hold the
po->bind_lockwhen testingpo->runningand rebinding. Else, it can race with other rebind operations, such as that inpacket_set_ringfrompacket_rcvtotpacket_rcv. Concurrent updates can result in a socket being added to a fanout group twice, causing use-after-free KASAN bug reports, among others.Reported independently by both trinity and syzkaller. Verified that the syzkaller reproducer passes after this patch.
I forgot to add the Fixes tag, sorry.
Quoted 2 lines
Reported-by nixioaming <[email protected]>Signed-off-by Willem de Bruijn <[email protected]>
David Miller
maintainerFrom: Willem de Bruijn <[email protected]>
Date: Fri, 15 Sep 2017 10:07:46 -0400
Quoted 14 lines · Willem de Bruijn
Quoted 13 lines
Packet socket bind operations must hold the
po->bind_lock. This keepspo->runningconsistent with whether the socket is actually on a ptype list to receive packets.
fanout_addunbinds a socket and its packet_rcv/tpacket_rcv call, then binds the fanout object to receive throughpacket_rcv_fanout.Make it hold the
po->bind_lockwhen testingpo->runningand rebinding. Else, it can race with other rebind operations, such as that inpacket_set_ringfrompacket_rcvtotpacket_rcv. Concurrent updates can result in a socket being added to a fanout group twice, causing use-after-free KASAN bug reports, among others.
Quoted 7 lines
Quoted 3 lines
Reported independently by both trinity and syzkaller. Verified that the syzkaller reproducer passes after this patch.
I forgot to add the Fixes tag, sorry.
Fixes dc99f600698d ("packet: Add fanout support.")
Applied and queued up for stable as it fixes this race and I can't see any new problems it introduces.
But boy is this one messy area.
The scariest thing to me now is the save/restore sequence done by packet_set_ring(), for example.
spin_lock(&po->bind_lock);
was_running = po->running;
num = po->num;
if (was_running) {
po->num = 0;
__unregister_prot_hook(sk, false);
}
spin_unlock(&po->bind_lock);
...
spin_lock(&po->bind_lock);
if (was_running) {
po->num = num;
register_prot_hook(sk);
}
spin_unlock(&po->bind_lock);The socket is also locked during this sequence but that doesn't prevent parallel changes to the running state.
Since po->bind_lock is dropped, it's possible for another thread to grab bind_lock and bind it meanwhile.
The above code seems to assume that can't happen, and that register_prot_hook() will always see po->running set to zero and rebind the socket.
If the race happens we'll have weird state, because we did not rebind yet we modified po->num.
We seem to have a hierachy of sleeping and non-sleeping locks that do not work well together.
Willem de Bruijn
maintainerQuoted 21 lines · David Miller
From: Willem de Bruijn <[email protected]>
Date: Fri, 15 Sep 2017 10:07:46 -0400Quoted 18 lines · Willem de Bruijn
Quoted 16 lines
Packet socket bind operations must hold the
po->bind_lock. This keepspo->runningconsistent with whether the socket is actually on a ptype list to receive packets.
fanout_addunbinds a socket and its packet_rcv/tpacket_rcv call, then binds the fanout object to receive throughpacket_rcv_fanout.Make it hold the
po->bind_lockwhen testingpo->runningand rebinding. Else, it can race with other rebind operations, such as that inpacket_set_ringfrompacket_rcvtotpacket_rcv. Concurrent updates can result in a socket being added to a fanout group twice, causing use-after-free KASAN bug reports, among others.Reported independently by both trinity and syzkaller. Verified that the syzkaller reproducer passes after this patch.
Quoted 8 lines
Quoted 3 lines
I forgot to add the Fixes tag, sorry.
Fixes dc99f600698d ("packet: Add fanout support.")Applied and queued up for stable as it fixes this race and I can't see any new problems it introduces.
But boy is this one messy area.
Yeah, I've been staring at this code for a while now. But I don't see an obvious way to simplify it. packet_notifier does not hold the socket lock, so even if all of bind, set_ring and fanout_add do hold that, bind_lock is still needed. packet_mmap may not take the socket lock, so pg_vec_lock must remain to synchronize with packet_set_ring even if for no other reason. I had a look at using rcu pointers for rx_ring and tx_ring, to avoid taking that lock in the datapath and possibly updating without the unbind/bind dance. But that update needs to be atomic with purging the socket queue, so the socket must be properly quiesced.
Quoted 25 lines
The scariest thing to me now is the save/restore sequence done by
packet_set_ring(), for example.spin_lock(&po->bind_lock); was_running = po->running; num = po->num; if (was_running) { po->num = 0; __unregister_prot_hook(sk, false); } spin_unlock(&po->bind_lock); ... spin_lock(&po->bind_lock); if (was_running) { po->num = num; register_prot_hook(sk); } spin_unlock(&po->bind_lock);The socket is also locked during this sequence but that doesn't prevent parallel changes to the running state.
Since
po->bind_lockis dropped, it's possible for another thread to grabbind_lockand bind it meanwhile.
Quoted 6 lines
The above code seems to assume that can't happen, and that
register_prot_hook()will always seepo->runningset to zero and rebind the socket.If the race happens we'll have weird state, because we did not rebind yet we modified
po->num.
It appears that the only path that may try to bind without holding the socket lock is packet_notifier. That skips register_prot_hook if !po->num.
Quoted 2 lines
We seem to have a hierachy of sleeping and non-sleeping locks that do not work well together.
Given the number recent bugs that were fixed by locking the socket inside a particular setsockopt case, I think that we should lock that entire function, similar to other protocol families (after verifying that all cases can indeed sleep).
Another issue that looks fragile is the test for po->fanout in packet_do_bind before taking the socket lock.
Quoted 38 lines · Willem de Bruijn
Packet socket bind operations must hold the
po->bind_lock. This keepspo->runningconsistent with whether the socket is actually on a ptype list to receive packets.
fanout_addunbinds a socket and its packet_rcv/tpacket_rcv call, then binds the fanout object to receive throughpacket_rcv_fanout.Make it hold the
po->bind_lockwhen testingpo->runningand rebinding. Else, it can race with other rebind operations, such as that inpacket_set_ringfrompacket_rcvtotpacket_rcv. Concurrent updates can result in a socket being added to a fanout group twice, causing use-after-free KASAN bug reports, among others.Reported independently by both trinity and syzkaller. Verified that the syzkaller reproducer passes after this patch.
Reported-by nixioaming <[email protected]>Signed-off-by Willem de Bruijn <[email protected]>--- net/packet/af_packet.c | 16 +++++++++++----- 1 file changed, 11 insertions(+), 5 deletions(-)net/packet/af_packet.c +0
-4@@ -1684,10 +1684,6 @@ static int fanout_add(struct sock *sk, u16 id, u16 type_flags)mutex_lock (&fanout_mutex); - err = -EINVAL ; -if (!po->running) - goto out; - err = -EALREADY ;if (po->fanout) goto out;
net/packet/af_packet.c static int fanout_add(struct sock *sk, u16 id, u16 type_flags) · line 1750
Quoted 7 lines
net/packet/af_packet.c +3
-1@@ -1749,7 +1745,10 @@ static int fanout_add(struct sock *sk, u16 id, u16 type_flags)list_add (&match->list, &fanout_list); } err = -EINVAL ; - if (match->type == type && + +spin_lock (&po->bind_lock); + if (po->running &&
As you move the po->running check later after setting po->rollover, I wonder if po->rollover possibly depends on po>running on other path?
Quoted 5 lines
net/packet/af_packet.c +1
-0+ match->type == type && match->prot_hook.type == po->prot_hook.type && match->prot_hook.dev == po->prot_hook.dev) { err = -ENOSPC ;
net/packet/af_packet.c static int fanout_add(struct sock *sk, u16 id, u16 type_flags) · line 1768
Quoted 9 lines
net/packet/af_packet.c +6
-0@@ -1761,6 +1760,13 @@ static int fanout_add(struct sock *sk, u16 id, u16 type_flags) err =0 ; } } +spin_unlock (&po->bind_lock); + +if (err && !refcount_read (&match->sk_ref)) { +list_del (&match->list); +kfree (match); + }
This looks correct but still seems odd, it smells you don't use refcnt in an expected way.
Willem de Bruijn
maintainerQuoted 19 lines
Quoted 19 lines
net/packet/af_packet.c +0
-4@@ -1684,10 +1684,6 @@ static int fanout_add(struct sock *sk, u16 id, u16 type_flags)mutex_lock (&fanout_mutex); - err = -EINVAL ; -if (!po->running) - goto out; - err = -EALREADY ;if (po->fanout) goto out;@@ -1749,7 +1745,10 @@ static int fanout_add(struct sock *sk, u16 id, u16 type_flags)list_add (&match->list, &fanout_list); } err = -EINVAL ;
Quoted 8 lines
net/packet/af_packet.c static int fanout_add(struct sock *sk, u16 id, u16 type_flags) · line 1750
Quoted 4 lines
net/packet/af_packet.c +3
-1- if (match->type == type && + +spin_lock (&po->bind_lock); + if (po->running &&As you move the
po->runningcheck later after settingpo->rollover, I wonder ifpo->rolloverpossibly depends on po>running on other path?
The rollover code does not explicitly check po->running, if that is what you are concerned about.
A newly allocated po->rollover structure will also only be accessed from the datapath once the socket is added to a fanout group. Both before and after this patch, that happens well after the po->running check.
If the socket already had a po->rollover, then it must already have had a po->fanout, too, so fanout_add does not reach this code.
Quoted 9 lines
Quoted 9 lines
net/packet/af_packet.c +2
-0+ match->type == type && match->prot_hook.type == po->prot_hook.type && match->prot_hook.dev == po->prot_hook.dev) { err = -ENOSPC ;@@ -1761,6 +1760,13 @@ static int fanout_add(struct sock *sk, u16 id, u16 type_flags) err =0 ; } } +spin_unlock (&po->bind_lock);
Quoted 8 lines
net/packet/af_packet.c static int fanout_add(struct sock *sk, u16 id, u16 type_flags) · line 1768
Quoted 5 lines
net/packet/af_packet.c +5
-0+ +if (err && !refcount_read (&match->sk_ref)) { +list_del (&match->list); +kfree (match); + }This looks correct but still seems odd, it smells you don't use refcnt in an expected way.
It tests whether the object has no references, in which case it must have been newly allocated and the fanout join operation must have failed.
I don't see an obviously simpler patch.
The entire code could perhaps be restructured eventually. There is, for instance, no reason to test match->{type, prot_hook.type, prot_hook.dev} when having just allocated the structure. Nor to test whether sk_ref exceeds PACKET_FANOUT_MAX. Conversely, this sk_ref == 0 test only makes sense when having taken the (!match) branch earlier.
But a refactor is out of scope for a bug fix.