Messages in this thread | | | From | Eric Dumazet <> | Date | Thu, 19 Jan 2023 05:16:13 +0100 | Subject | Re: [PATCH] L2CAP: Fix null-ptr-deref in l2cap_sock_set_shutdown_cb |
| |
On Thu, Jan 19, 2023 at 2:35 AM Sungwoo Kim <iam@sung-woo.kim> wrote: > > The L2CAP socket shutdown invokes l2cap_sock_destruct without a lock > on conn->chan_lock, assigning NULL to chan->data *just before* > the l2cap_disconnect_req thread that accesses to chan->data.
This is racy then ?
> This patch prevent it by adding a null check for a workaround, instead > of fixing a lock.
This would at least require some barriers I think.
What about other _cb helpers also reading/using chan->data ?
> > This bug is found by FuzzBT, a modified Syzkaller by Sungwoo Kim(me). > Ruoyu Wu(wuruoyu@me.com) and Hui Peng(benquike@gmail.com) has helped > the FuzzBT project. > > Signed-off-by: Sungwoo Kim <iam@sung-woo.kim>
I would also add
Fixes: 1bff51ea59a9 ("Bluetooth: fix use-after-free error in lock_sock_nested()")
> --- > net/bluetooth/l2cap_sock.c | 8 +++++--- > 1 file changed, 5 insertions(+), 3 deletions(-) > > diff --git a/net/bluetooth/l2cap_sock.c b/net/bluetooth/l2cap_sock.c > index ca8f07f35..350c7afdf 100644 > --- a/net/bluetooth/l2cap_sock.c > +++ b/net/bluetooth/l2cap_sock.c > @@ -1681,9 +1681,11 @@ static void l2cap_sock_set_shutdown_cb(struct l2cap_chan *chan) > { > struct sock *sk = chan->data; >
Other similar fixes simply do:
if (!sk) return;
I would chose to use the same coding style in net/bluetooth/l2cap_sock.c
> - lock_sock(sk); > - sk->sk_shutdown = SHUTDOWN_MASK; > - release_sock(sk); > + if (!sk) { > + lock_sock(sk); > + sk->sk_shutdown = SHUTDOWN_MASK; > + release_sock(sk); > + } > } > > static long l2cap_sock_get_sndtimeo_cb(struct l2cap_chan *chan) > -- > 2.25.1 >
| |