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

Luiz Augusto von Dentz
2024-06-11T19:25:06Z

Hi Edward,

On Tue, Jun 11, 2024 at 10:50 AM Edward Adam Davis <eadavis@qq.com> 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@syzkaller.appspotmail.com
> Signed-off-by: Edward Adam Davis <eadavis@qq.com>
> ---
> 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