Hi Edward, On Tue, Jun 11, 2024 at 10:50 AM Edward Adam Davis <eadavis@xxxxxx> wrote: > > The problem occurs between the system call to close the sock and hci_rx_work, > where the former releases the sock and the latter accesses it without lock protection. > > CPU0 CPU1 > ---- ---- > sock_close hci_rx_work > l2cap_sock_release hci_acldata_packet > l2cap_sock_kill l2cap_recv_frame > sk_free l2cap_conless_channel > l2cap_sock_recv_cb > > If hci_rx_work processes the data that needs to be received before the sock is > closed, then everything is normal; Otherwise, the work thread may access the > released sock when receiving data. > > Add a chan mutex in the rx callback of the sock to achieve synchronization between > the sock release and recv cb. > > Reported-and-tested-by: syzbot+b7f6f8c9303466e16c8a@xxxxxxxxxxxxxxxxxxxxxxxxx > Signed-off-by: Edward Adam Davis <eadavis@xxxxxx> > --- > net/bluetooth/l2cap_sock.c | 20 +++++++++++++++++--- > 1 file changed, 17 insertions(+), 3 deletions(-) > > diff --git a/net/bluetooth/l2cap_sock.c b/net/bluetooth/l2cap_sock.c > index 6db60946c627..f3e9236293e1 100644 > --- a/net/bluetooth/l2cap_sock.c > +++ b/net/bluetooth/l2cap_sock.c > @@ -1413,6 +1413,8 @@ static int l2cap_sock_release(struct socket *sock) > l2cap_chan_hold(chan); > l2cap_chan_lock(chan); > > + if (refcount_read(&sk->sk_refcnt) == 1) > + chan->data = NULL; Might be a good idea to add some comment on why checking for refcount == 1 is the right thing to do here, or perhaps we can always assign chan->data to NULL, instead of that perhaps we could have it done in l2cap_sock_kill? > sock_orphan(sk); > l2cap_sock_kill(sk); > > @@ -1481,12 +1483,22 @@ static struct l2cap_chan *l2cap_sock_new_connection_cb(struct l2cap_chan *chan) > > static int l2cap_sock_recv_cb(struct l2cap_chan *chan, struct sk_buff *skb) > { > - struct sock *sk = chan->data; > - struct l2cap_pinfo *pi = l2cap_pi(sk); > + struct sock *sk; > + struct l2cap_pinfo *pi; > int err; > > - lock_sock(sk); > + l2cap_chan_hold(chan); > + l2cap_chan_lock(chan); > + sk = chan->data; > + > + if (!sk) { > + l2cap_chan_unlock(chan); > + l2cap_chan_put(chan); > + return -ENXIO; > + } > > + pi = l2cap_pi(sk); > + lock_sock(sk); > if (chan->mode == L2CAP_MODE_ERTM && !list_empty(&pi->rx_busy)) { > err = -ENOMEM; > goto done; > @@ -1535,6 +1547,8 @@ static int l2cap_sock_recv_cb(struct l2cap_chan *chan, struct sk_buff *skb) > > done: > release_sock(sk); > + l2cap_chan_unlock(chan); > + l2cap_chan_put(chan); > > return err; > } > -- > 2.43.0 > -- Luiz Augusto von Dentz