Re: [PATCH] bluetooth/l2cap: sync sock recv cb and release

[Edward Adam Davis]

Hi Luiz Augusto von Dentz,

On Tue, 11 Jun 2024 15:24:52 -0400, Luiz Augusto von Dentz 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?
In this case, it is possible to always set chan->data to NULL, but I think a
better approach would be to release sock in l2cap_sock_kill when the reference
count of the sock is 1. Previously, it was mentioned that setting chan->data to
NULL is more rigorous.
And chan->data is bound one-on-one to the sock, if the sock is not released,
I can't confirm whether setting chan->data to NULL will affect the operation of
the sock on other paths.
> 
> >         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;
> >  }

--
Edward