diff mbox series

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

Message ID tencent_8D9918CF808DC6A5FF8DCB12055DECDDF107@qq.com
State Accepted
Commit e3203b17771757fdcd259d6378673f1590e36694
Headers show
Series [v2] bluetooth/l2cap: sync sock recv cb and release | expand

Commit Message

Edward Adam Davis June 15, 2024, 1:45 a.m. UTC
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.

Sock is dead, so set chan data to NULL, avoid others use invalid sock pointer.

Reported-and-tested-by: syzbot+b7f6f8c9303466e16c8a@syzkaller.appspotmail.com
Signed-off-by: Edward Adam Davis <eadavis@qq.com>
---
 net/bluetooth/l2cap_sock.c | 25 ++++++++++++++++++++++---
 1 file changed, 22 insertions(+), 3 deletions(-)

Comments

patchwork-bot+bluetooth@kernel.org June 17, 2024, 7 p.m. UTC | #1
Hello:

This patch was applied to bluetooth/bluetooth-next.git (master)
by Luiz Augusto von Dentz <luiz.von.dentz@intel.com>:

On Sat, 15 Jun 2024 09:45:54 +0800 you 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
> 
> [...]

Here is the summary with links:
  - [v2] bluetooth/l2cap: sync sock recv cb and release
    https://git.kernel.org/bluetooth/bluetooth-next/c/e3203b177717

You are awesome, thank you!
Luiz Augusto von Dentz June 20, 2024, 4:53 p.m. UTC | #2
Hi Edward,

On Fri, Jun 14, 2024 at 9:46 PM 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.
>
> Sock is dead, so set chan data to NULL, avoid others use invalid sock pointer.
>
> Reported-and-tested-by: syzbot+b7f6f8c9303466e16c8a@syzkaller.appspotmail.com
> Signed-off-by: Edward Adam Davis <eadavis@qq.com>
> ---
>  net/bluetooth/l2cap_sock.c | 25 ++++++++++++++++++++++---
>  1 file changed, 22 insertions(+), 3 deletions(-)
>
> diff --git a/net/bluetooth/l2cap_sock.c b/net/bluetooth/l2cap_sock.c
> index 6db60946c627..f45cdf9bc985 100644
> --- a/net/bluetooth/l2cap_sock.c
> +++ b/net/bluetooth/l2cap_sock.c
> @@ -1239,6 +1239,10 @@ static void l2cap_sock_kill(struct sock *sk)
>
>         BT_DBG("sk %p state %s", sk, state_to_string(sk->sk_state));
>
> +       /* Sock is dead, so set chan data to NULL, avoid other task use invalid
> +        * sock pointer.
> +        */
> +       l2cap_pi(sk)->chan->data = NULL;
>         /* Kill poor orphan */
>
>         l2cap_chan_put(l2cap_pi(sk)->chan);
> @@ -1481,12 +1485,25 @@ 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);
> +       /* To avoid race with sock_release, a chan lock needs to be added here
> +        * to synchronize the sock.
> +        */
> +       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 +1552,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

Looks like this was never really tested properly:

============================================
WARNING: possible recursive locking detected
6.10.0-rc3-g4029dba6b6f1 #6823 Not tainted
--------------------------------------------
kworker/u5:0/35 is trying to acquire lock:
ffff888002ec2510 (&chan->lock#2/1){+.+.}-{3:3}, at:
l2cap_sock_recv_cb+0x44/0x1e0

but task is already holding lock:
ffff888002ec2510 (&chan->lock#2/1){+.+.}-{3:3}, at:
l2cap_get_chan_by_scid+0xaf/0xd0

other info that might help us debug this:
 Possible unsafe locking scenario:

       CPU0
       ----
  lock(&chan->lock#2/1);
  lock(&chan->lock#2/1);

 *** DEADLOCK ***

 May be due to missing lock nesting notation

3 locks held by kworker/u5:0/35:
 #0: ffff888002b8a940 ((wq_completion)hci0#2){+.+.}-{0:0}, at:
process_one_work+0x750/0x930
 #1: ffff888002c67dd0 ((work_completion)(&hdev->rx_work)){+.+.}-{0:0},
at: process_one_work+0x44e/0x930
 #2: ffff888002ec2510 (&chan->lock#2/1){+.+.}-{3:3}, at:
l2cap_get_chan_by_scid+0xaf/0xd0

l2cap_sock_recv_cb is assumed to be called with the chan_lock held so
perhaps we can just do:

       sk = chan->data;
       if (!sk)
               return -ENXIO;
Edward Adam Davis June 21, 2024, 2:45 p.m. UTC | #3
Hi Luiz Augusto von Dentz,

On Thu, 20 Jun 2024 12:53:19 -0400, Luiz Augusto von Dentz wrote:
> >         release_sock(sk);
> > +       l2cap_chan_unlock(chan);
> > +       l2cap_chan_put(chan);
> >
> >         return err;
> >  }
> > --
> > 2.43.0
> 
> Looks like this was never really tested properly:
> 
> ============================================
> WARNING: possible recursive locking detected
> 6.10.0-rc3-g4029dba6b6f1 #6823 Not tainted
> --------------------------------------------
> kworker/u5:0/35 is trying to acquire lock:
> ffff888002ec2510 (&chan->lock#2/1){+.+.}-{3:3}, at:
> l2cap_sock_recv_cb+0x44/0x1e0
> 
> but task is already holding lock:
> ffff888002ec2510 (&chan->lock#2/1){+.+.}-{3:3}, at:
> l2cap_get_chan_by_scid+0xaf/0xd0
> 
> other info that might help us debug this:
>  Possible unsafe locking scenario:
> 
>        CPU0
>        ----
>   lock(&chan->lock#2/1);
>   lock(&chan->lock#2/1);
> 
>  *** DEADLOCK ***
> 
>  May be due to missing lock nesting notation
> 
> 3 locks held by kworker/u5:0/35:
>  #0: ffff888002b8a940 ((wq_completion)hci0#2){+.+.}-{0:0}, at:
> process_one_work+0x750/0x930
>  #1: ffff888002c67dd0 ((work_completion)(&hdev->rx_work)){+.+.}-{0:0},
> at: process_one_work+0x44e/0x930
>  #2: ffff888002ec2510 (&chan->lock#2/1){+.+.}-{3:3}, at:
> l2cap_get_chan_by_scid+0xaf/0xd0
> 
> l2cap_sock_recv_cb is assumed to be called with the chan_lock held so
> perhaps we can just do:
> 
>        sk = chan->data;
>        if (!sk)
>                return -ENXIO;

If the release occurs after this judgment, the same problem will still occur. 
Recv and release must be synchronized using locks, which can be solved by
adding new lock.

Can you provide a reproduction program for the AA lock mentioned above?

--
Edward
Pauli Virtanen June 21, 2024, 3:57 p.m. UTC | #4
Hi,

pe, 2024-06-21 kello 22:45 +0800, Edward Adam Davis kirjoitti:
> Hi Luiz Augusto von Dentz,
> 
> On Thu, 20 Jun 2024 12:53:19 -0400, Luiz Augusto von Dentz wrote:
> > >         release_sock(sk);
> > > +       l2cap_chan_unlock(chan);
> > > +       l2cap_chan_put(chan);
> > > 
> > >         return err;
> > >  }
> > > --
> > > 2.43.0
> > 
> > Looks like this was never really tested properly:
> > 
> > ============================================
> > WARNING: possible recursive locking detected
> > 6.10.0-rc3-g4029dba6b6f1 #6823 Not tainted
> > --------------------------------------------
> > kworker/u5:0/35 is trying to acquire lock:
> > ffff888002ec2510 (&chan->lock#2/1){+.+.}-{3:3}, at:
> > l2cap_sock_recv_cb+0x44/0x1e0
> > 
> > but task is already holding lock:
> > ffff888002ec2510 (&chan->lock#2/1){+.+.}-{3:3}, at:
> > l2cap_get_chan_by_scid+0xaf/0xd0
> > 
> > other info that might help us debug this:
> >  Possible unsafe locking scenario:
> > 
> >        CPU0
> >        ----
> >   lock(&chan->lock#2/1);
> >   lock(&chan->lock#2/1);
> > 
> >  *** DEADLOCK ***
> > 
> >  May be due to missing lock nesting notation
> > 
> > 3 locks held by kworker/u5:0/35:
> >  #0: ffff888002b8a940 ((wq_completion)hci0#2){+.+.}-{0:0}, at:
> > process_one_work+0x750/0x930
> >  #1: ffff888002c67dd0 ((work_completion)(&hdev->rx_work)){+.+.}-{0:0},
> > at: process_one_work+0x44e/0x930
> >  #2: ffff888002ec2510 (&chan->lock#2/1){+.+.}-{3:3}, at:
> > l2cap_get_chan_by_scid+0xaf/0xd0
> > 
> > l2cap_sock_recv_cb is assumed to be called with the chan_lock held so
> > perhaps we can just do:
> > 
> >        sk = chan->data;
> >        if (!sk)
> >                return -ENXIO;
> 
> If the release occurs after this judgment, the same problem will still occur. 
> Recv and release must be synchronized using locks, which can be solved by
> adding new lock.
> 
> Can you provide a reproduction program for the AA lock mentioned above?

eg. l2cap_data_channel() calls l2cap_sock_recv_cb with chan->lock (and
conn->chan_lock) held.

All callsites eg. l2cap_raw_recv() don't seem to hold chan->lock, so
you'd probably need to check the locking on all of them, if you want to
use chan->lock for this synchronization.
Edward Adam Davis June 22, 2024, 3:46 a.m. UTC | #5
Hi Luiz Augusto von Dentz,

On Thu, 20 Jun 2024 12:53:19 -0400, Luiz Augusto von Dentz wrote:
> >         release_sock(sk);
> > +       l2cap_chan_unlock(chan);
> > +       l2cap_chan_put(chan);
> >
> >         return err;
> >  }
> > --
> > 2.43.0
> 
> Looks like this was never really tested properly:
> 
> ============================================
> WARNING: possible recursive locking detected
> 6.10.0-rc3-g4029dba6b6f1 #6823 Not tainted
> --------------------------------------------
> kworker/u5:0/35 is trying to acquire lock:
> ffff888002ec2510 (&chan->lock#2/1){+.+.}-{3:3}, at:
> l2cap_sock_recv_cb+0x44/0x1e0
> 
> but task is already holding lock:
> ffff888002ec2510 (&chan->lock#2/1){+.+.}-{3:3}, at:
> l2cap_get_chan_by_scid+0xaf/0xd0
> 
> other info that might help us debug this:
>  Possible unsafe locking scenario:
> 
>        CPU0
>        ----
>   lock(&chan->lock#2/1);
>   lock(&chan->lock#2/1);
> 
>  *** DEADLOCK ***
> 
>  May be due to missing lock nesting notation
> 
> 3 locks held by kworker/u5:0/35:
>  #0: ffff888002b8a940 ((wq_completion)hci0#2){+.+.}-{0:0}, at:
> process_one_work+0x750/0x930
>  #1: ffff888002c67dd0 ((work_completion)(&hdev->rx_work)){+.+.}-{0:0},
> at: process_one_work+0x44e/0x930
>  #2: ffff888002ec2510 (&chan->lock#2/1){+.+.}-{3:3}, at:
> l2cap_get_chan_by_scid+0xaf/0xd0
> 
> l2cap_sock_recv_cb is assumed to be called with the chan_lock held so
> perhaps we can just do:
> 
>        sk = chan->data;
>        if (!sk)
>                return -ENXIO;

If the release occurs after this judgment, the same problem will still occur. 
Recv and release must be synchronized using locks, which can be solved by
adding new lock.

Please use the new patch https://syzkaller.appspot.com/text?tag=Patch&x=15d2c48e980000, I have tested in 
https://syzkaller.appspot.com/bug?extid=b7f6f8c9303466e16c8a

--
Edward
Luiz Augusto von Dentz June 24, 2024, 1:36 p.m. UTC | #6
Hi Edward,

On Fri, Jun 21, 2024 at 11:46 PM Edward Adam Davis <eadavis@qq.com> wrote:
>
> Hi Luiz Augusto von Dentz,
>
> On Thu, 20 Jun 2024 12:53:19 -0400, Luiz Augusto von Dentz wrote:
> > >         release_sock(sk);
> > > +       l2cap_chan_unlock(chan);
> > > +       l2cap_chan_put(chan);
> > >
> > >         return err;
> > >  }
> > > --
> > > 2.43.0
> >
> > Looks like this was never really tested properly:
> >
> > ============================================
> > WARNING: possible recursive locking detected
> > 6.10.0-rc3-g4029dba6b6f1 #6823 Not tainted
> > --------------------------------------------
> > kworker/u5:0/35 is trying to acquire lock:
> > ffff888002ec2510 (&chan->lock#2/1){+.+.}-{3:3}, at:
> > l2cap_sock_recv_cb+0x44/0x1e0
> >
> > but task is already holding lock:
> > ffff888002ec2510 (&chan->lock#2/1){+.+.}-{3:3}, at:
> > l2cap_get_chan_by_scid+0xaf/0xd0
> >
> > other info that might help us debug this:
> >  Possible unsafe locking scenario:
> >
> >        CPU0
> >        ----
> >   lock(&chan->lock#2/1);
> >   lock(&chan->lock#2/1);
> >
> >  *** DEADLOCK ***
> >
> >  May be due to missing lock nesting notation
> >
> > 3 locks held by kworker/u5:0/35:
> >  #0: ffff888002b8a940 ((wq_completion)hci0#2){+.+.}-{0:0}, at:
> > process_one_work+0x750/0x930
> >  #1: ffff888002c67dd0 ((work_completion)(&hdev->rx_work)){+.+.}-{0:0},
> > at: process_one_work+0x44e/0x930
> >  #2: ffff888002ec2510 (&chan->lock#2/1){+.+.}-{3:3}, at:
> > l2cap_get_chan_by_scid+0xaf/0xd0
> >
> > l2cap_sock_recv_cb is assumed to be called with the chan_lock held so
> > perhaps we can just do:
> >
> >        sk = chan->data;
> >        if (!sk)
> >                return -ENXIO;
>
> If the release occurs after this judgment, the same problem will still occur.
> Recv and release must be synchronized using locks, which can be solved by
> adding new lock.
>
> Please use the new patch https://syzkaller.appspot.com/text?tag=Patch&x=15d2c48e980000, I have tested in
> https://syzkaller.appspot.com/bug?extid=b7f6f8c9303466e16c8a

Hmm, why don't we just fix l2cap_conless_channel? Btw,
l2cap_conless_channel is normally not used by any profiles thus why
there isn't any CI covering it, on the other hand l2cap_data_channel
is used by 99% of the profiles.

> --
> Edward
>
Luiz Augusto von Dentz June 25, 2024, 3:08 a.m. UTC | #7
Hi Edward,

On Mon, Jun 24, 2024 at 8:53 PM Edward Adam Davis <eadavis@qq.com> wrote:
>
> Hi Luiz Augusto von Dentz,
>
> On Mon, 24 Jun 2024 09:36:14 -0400, Luiz Augusto von Dentz wrote:
> > > > Looks like this was never really tested properly:
> > > >
> > > > ============================================
> > > > WARNING: possible recursive locking detected
> > > > 6.10.0-rc3-g4029dba6b6f1 #6823 Not tainted
> > > > --------------------------------------------
> > > > kworker/u5:0/35 is trying to acquire lock:
> > > > ffff888002ec2510 (&chan->lock#2/1){+.+.}-{3:3}, at:
> > > > l2cap_sock_recv_cb+0x44/0x1e0
> > > >
> > > > but task is already holding lock:
> > > > ffff888002ec2510 (&chan->lock#2/1){+.+.}-{3:3}, at:
> > > > l2cap_get_chan_by_scid+0xaf/0xd0
> > > >
> > > > other info that might help us debug this:
> > > >  Possible unsafe locking scenario:
> > > >
> > > >        CPU0
> > > >        ----
> > > >   lock(&chan->lock#2/1);
> > > >   lock(&chan->lock#2/1);
> > > >
> > > >  *** DEADLOCK ***
> > > >
> > > >  May be due to missing lock nesting notation
> > > >
> > > > 3 locks held by kworker/u5:0/35:
> > > >  #0: ffff888002b8a940 ((wq_completion)hci0#2){+.+.}-{0:0}, at:
> > > > process_one_work+0x750/0x930
> > > >  #1: ffff888002c67dd0 ((work_completion)(&hdev->rx_work)){+.+.}-{0:0},
> > > > at: process_one_work+0x44e/0x930
> > > >  #2: ffff888002ec2510 (&chan->lock#2/1){+.+.}-{3:3}, at:
> > > > l2cap_get_chan_by_scid+0xaf/0xd0
> > > >
> > > > l2cap_sock_recv_cb is assumed to be called with the chan_lock held so
> > > > perhaps we can just do:
> > > >
> > > >        sk = chan->data;
> > > >        if (!sk)
> > > >                return -ENXIO;
> > >
> > > If the release occurs after this judgment, the same problem will still occur.
> > > Recv and release must be synchronized using locks, which can be solved by
> > > adding new lock.
> > >
> > > Please use the new patch https://syzkaller.appspot.com/text?tag=Patch&x=15d2c48e980000, I have tested in
> > > https://syzkaller.appspot.com/bug?extid=b7f6f8c9303466e16c8a
> >
> > Hmm, why don't we just fix l2cap_conless_channel? Btw,
> > l2cap_conless_channel is normally not used by any profiles thus why
> > there isn't any CI covering it, on the other hand l2cap_data_channel
> > is used by 99% of the profiles.
> Yes, we can fix l2cap_conless_channel, but key point is that "chan->lock"
> cannot be used to synchronize l2cap_conless_channel and l2cap_sock_release,
> otherwise it will form an AA lock with l2cap_data_channel.

Don't follow, what l2cap_conless_channel has to do with
l2cap_data_channel? You can't have the same channel calling both, it
is either connectionless or it is not.

> Why not fix it in l2cap_conless_channel but in l2cap_sock_recv_cb, because
> l2cap_sock_recv_cb is on the same layer as l2cap_sock_kill, using a new
> lock for synchronization is more appropriate.

There are dedicated locks for both layers and now that we are doing
chan->lock for before chan->recv the locking sequence should be the
same in all instances.

> --
> Edward
>
diff mbox series

Patch

diff --git a/net/bluetooth/l2cap_sock.c b/net/bluetooth/l2cap_sock.c
index 6db60946c627..f45cdf9bc985 100644
--- a/net/bluetooth/l2cap_sock.c
+++ b/net/bluetooth/l2cap_sock.c
@@ -1239,6 +1239,10 @@  static void l2cap_sock_kill(struct sock *sk)
 
 	BT_DBG("sk %p state %s", sk, state_to_string(sk->sk_state));
 
+	/* Sock is dead, so set chan data to NULL, avoid other task use invalid
+	 * sock pointer.
+	 */
+	l2cap_pi(sk)->chan->data = NULL;
 	/* Kill poor orphan */
 
 	l2cap_chan_put(l2cap_pi(sk)->chan);
@@ -1481,12 +1485,25 @@  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);
+	/* To avoid race with sock_release, a chan lock needs to be added here
+	 * to synchronize the sock.
+	 */
+	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 +1552,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;
 }