Received-SPF: pass (google.com: domain of linux-bluetooth+bounces-5260-linux.lists.archive=gmail.com@vger.kernel.org designates 2604:1380:4601:e00::3 as permitted sender) client-ip=2604:1380:4601:e00::3;
Precedence: bulk
MIME-Version: 1.0
References: <000000000000b0906d061a468b93@google.com> <tencent_0CCE4C90A7C306FCD2EE466AC9882EFFAE06@qq.com>
In-Reply-To: <tencent_0CCE4C90A7C306FCD2EE466AC9882EFFAE06@qq.com>
From: Luiz Augusto von Dentz <luiz.dentz@gmail.com>
Date: Tue, 11 Jun 2024 15:24:52 -0400
Message-ID: <CABBYNZKArASNApkJPvJn8C4HE0y5aZy0X2ZrOjjdrsDhBO+7rQ@mail.gmail.com>
Subject: Re: [PATCH] bluetooth/l2cap: sync sock recv cb and release
To: Edward Adam Davis <eadavis@qq.com>
Cc: syzbot+b7f6f8c9303466e16c8a@syzkaller.appspotmail.com, 
	johan.hedberg@gmail.com, linux-bluetooth@vger.kernel.org, 
	linux-kernel@vger.kernel.org, marcel@holtmann.org, 
	syzkaller-bugs@googlegroups.com
Content-Type: text/plain; charset="UTF-8"
Content-Transfer-Encoding: quoted-printable

Hi Edward,

On Tue, Jun 11, 2024 at 10:50=E2=80=AFAM Edward Adam Davis <eadavis@qq.com>=
 wrote:
>
> The problem occurs between the system call to close the sock and hci_rx_w=
ork,
> where the former releases the sock and the latter accesses it without loc=
k 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 so=
ck 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 synchronizatio=
n 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) =3D=3D 1)
> +               chan->data =3D NULL;

Might be a good idea to add some comment on why checking for refcount
=3D=3D 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_connecti=
on_cb(struct l2cap_chan *chan)
>
>  static int l2cap_sock_recv_cb(struct l2cap_chan *chan, struct sk_buff *s=
kb)
>  {
> -       struct sock *sk =3D chan->data;
> -       struct l2cap_pinfo *pi =3D l2cap_pi(sk);
> +       struct sock *sk;
> +       struct l2cap_pinfo *pi;
>         int err;
>
> -       lock_sock(sk);
> +       l2cap_chan_hold(chan);
> +       l2cap_chan_lock(chan);
> +       sk =3D chan->data;
> +
> +       if (!sk) {
> +               l2cap_chan_unlock(chan);
> +               l2cap_chan_put(chan);
> +               return -ENXIO;
> +       }
>
> +       pi =3D l2cap_pi(sk);
> +       lock_sock(sk);
>         if (chan->mode =3D=3D L2CAP_MODE_ERTM && !list_empty(&pi->rx_busy=
)) {
>                 err =3D -ENOMEM;
>                 goto done;
> @@ -1535,6 +1547,8 @@ static int l2cap_sock_recv_cb(struct l2cap_chan *ch=
an, struct sk_buff *skb)
>
>  done:
>         release_sock(sk);
> +       l2cap_chan_unlock(chan);
> +       l2cap_chan_put(chan);
>
>         return err;
>  }
> --
> 2.43.0
>


--=20
Luiz Augusto von Dentz