[PATCH] L2CAP: Fix null-ptr-deref in l2cap_sock_set_shutdown_cb

[PATCH] L2CAP: Fix null-ptr-deref in l2cap_sock_set_shutdown_cb

[Sungwoo Kim]

The L2CAP socket shutdown invokes l2cap_sock_destruct without a lock
on conn->chan_lock, assigning NULL to chan->data *just before*
the l2cap_disconnect_req thread that accesses to chan->data.
This patch prevent it by adding a null check for a workaround, instead
of fixing a lock.

This bug is found by FuzzBT, a modified Syzkaller by Sungwoo Kim(me).
Ruoyu Wu(wuruoyu@me.com) and Hui Peng(benquike@gmail.com) has helped
the FuzzBT project.

Signed-off-by: Sungwoo Kim <iam@sung-woo.kim>
---
 net/bluetooth/l2cap_sock.c | 8 +++++---
 1 file changed, 5 insertions(+), 3 deletions(-)

diff --git a/net/bluetooth/l2cap_sock.c b/net/bluetooth/l2cap_sock.c
index ca8f07f35..350c7afdf 100644
--- a/net/bluetooth/l2cap_sock.c
+++ b/net/bluetooth/l2cap_sock.c
@@ -1681,9 +1681,11 @@ static void l2cap_sock_set_shutdown_cb(struct l2cap_chan *chan)
 {
 	struct sock *sk = chan->data;
 
-	lock_sock(sk);
-	sk->sk_shutdown = SHUTDOWN_MASK;
-	release_sock(sk);
+	if (!sk) {
+		lock_sock(sk);
+		sk->sk_shutdown = SHUTDOWN_MASK;
+		release_sock(sk);
+	}
 }
 
 static long l2cap_sock_get_sndtimeo_cb(struct l2cap_chan *chan)
-- 
2.25.1

BUG: KASAN: null-ptr-deref in _raw_spin_lock_bh+0x4c/0xc0

[Sungwoo Kim]

Write of size 4 at addr 0000000000000098 by task kworker/u3:0/76

CPU: 0 PID: 76 Comm: kworker/u3:0 Not tainted 6.1.0-rc2 #129
Hardware name: QEMU Standard PC (i440FX + PIIX, 1996), BIOS 1.13.0-1ubuntu1.1 04/01/2014
Workqueue: hci0 hci_rx_work
Call Trace:
 <TASK>
 dump_stack_lvl+0x7b/0xb3
 print_report+0xed/0x200
 ? __virt_addr_valid+0x5c/0x240
 ? kasan_addr_to_slab+0xd/0xa0
 ? _raw_spin_lock_bh+0x4c/0xc0
 kasan_report+0xd3/0x100
 ? _raw_spin_lock_bh+0x4c/0xc0
 kasan_check_range+0x2d3/0x310
 __kasan_check_write+0x14/0x20
 _raw_spin_lock_bh+0x4c/0xc0
 lock_sock_nested+0x3f/0x160
 ? queue_work_on+0x90/0xd0
 l2cap_sock_set_shutdown_cb+0x3d/0x60
 l2cap_disconnect_req+0x1e3/0x2e0
 l2cap_bredr_sig_cmd+0x3d2/0x5ec0
 ? vprintk_emit+0x29b/0x4d0
 ? vprintk_default+0x2b/0x30
 ? vprintk+0xdc/0x100
 ? _printk+0x67/0x85
 ? bt_err+0x7f/0xc0
 ? bt_err+0x9a/0xc0
 l2cap_recv_frame+0x7bc/0x4e10
 ? _printk+0x67/0x85
 ? bt_err+0x7f/0xc0
 ? __wake_up_klogd+0xc4/0xf0
 l2cap_recv_acldata+0x327/0x650
 ? hci_conn_enter_active_mode+0x1b7/0x1f0
 hci_rx_work+0x6b7/0x7c0
 process_one_work+0x461/0xaf0
 worker_thread+0x5f8/0xba0
 kthread+0x1c5/0x200
 ? process_one_work+0xaf0/0xaf0
 ? kthread_blkcg+0x90/0x90
 ret_from_fork+0x1f/0x30
 </TASK>

[PATCH] L2CAP: Fix null-ptr-deref in l2cap_sock_set_shutdown_cb

[Sungwoo Kim]

Fix a critical typo on the prev patch - Sorry!

Signed-off-by: Sungwoo Kim <iam@sung-woo.kim>
---
 net/bluetooth/l2cap_sock.c | 8 +++++---
 1 file changed, 5 insertions(+), 3 deletions(-)

diff --git a/net/bluetooth/l2cap_sock.c b/net/bluetooth/l2cap_sock.c
index ca8f07f35..b9381d45d 100644
--- a/net/bluetooth/l2cap_sock.c
+++ b/net/bluetooth/l2cap_sock.c
@@ -1681,9 +1681,11 @@ static void l2cap_sock_set_shutdown_cb(struct l2cap_chan *chan)
 {
 	struct sock *sk = chan->data;
 
-	lock_sock(sk);
-	sk->sk_shutdown = SHUTDOWN_MASK;
-	release_sock(sk);
+	if (sk) {
+		lock_sock(sk);
+		sk->sk_shutdown = SHUTDOWN_MASK;
+		release_sock(sk);
+	}
 }
 
 static long l2cap_sock_get_sndtimeo_cb(struct l2cap_chan *chan)
-- 
2.25.1

RE: L2CAP: Fix null-ptr-deref in l2cap_sock_set_shutdown_cb

[bluez.test.bot]

[-- Attachment #1: Type: text/plain, Size: 1597 bytes --]

This is automated email and please do not reply to this email!

Dear submitter,

Thank you for submitting the patches to the linux bluetooth mailing list.
This is a CI test results with your patch series:
PW Link:https://patchwork.kernel.org/project/bluetooth/list/?series=713481

---Test result---

Test Summary:
CheckPatch                    PASS      1.94 seconds
GitLint                       PASS      0.34 seconds
SubjectPrefix                 FAIL      0.53 seconds
BuildKernel                   PASS      38.21 seconds
CheckAllWarning               PASS      41.62 seconds
CheckSparse                   PASS      46.12 seconds
CheckSmatch                   PASS      124.89 seconds
BuildKernel32                 PASS      36.50 seconds
TestRunnerSetup               PASS      513.51 seconds
TestRunner_l2cap-tester       PASS      18.77 seconds
TestRunner_iso-tester         PASS      19.40 seconds
TestRunner_bnep-tester        PASS      6.99 seconds
TestRunner_mgmt-tester        PASS      123.49 seconds
TestRunner_rfcomm-tester      PASS      10.74 seconds
TestRunner_sco-tester         PASS      9.58 seconds
TestRunner_ioctl-tester       PASS      10.96 seconds
TestRunner_mesh-tester        PASS      8.25 seconds
TestRunner_smp-tester         PASS      9.34 seconds
TestRunner_userchan-tester    PASS      6.94 seconds
IncrementalBuild              PASS      34.93 seconds

Details
##############################
Test: SubjectPrefix - FAIL
Desc: Check subject contains "Bluetooth" prefix
Output:
"Bluetooth: " prefix is not specified in the subject


---
Regards,
Linux Bluetooth

RE: L2CAP: Fix null-ptr-deref in l2cap_sock_set_shutdown_cb

[bluez.test.bot]

[-- Attachment #1: Type: text/plain, Size: 1599 bytes --]

This is automated email and please do not reply to this email!

Dear submitter,

Thank you for submitting the patches to the linux bluetooth mailing list.
This is a CI test results with your patch series:
PW Link:https://patchwork.kernel.org/project/bluetooth/list/?series=713488

---Test result---

Test Summary:
CheckPatch                    PASS      1.74 seconds
GitLint                       PASS      0.27 seconds
SubjectPrefix                 FAIL      0.45 seconds
BuildKernel                   PASS      43.01 seconds
CheckAllWarning               PASS      46.26 seconds
CheckSparse                   PASS      51.28 seconds
CheckSmatch                   PASS      136.55 seconds
BuildKernel32                 PASS      40.25 seconds
TestRunnerSetup               PASS      579.35 seconds
TestRunner_l2cap-tester       PASS      20.05 seconds
TestRunner_iso-tester         PASS      21.75 seconds
TestRunner_bnep-tester        PASS      7.20 seconds
TestRunner_mgmt-tester        PASS      136.12 seconds
TestRunner_rfcomm-tester      PASS      11.25 seconds
TestRunner_sco-tester         PASS      10.28 seconds
TestRunner_ioctl-tester       PASS      12.13 seconds
TestRunner_mesh-tester        PASS      9.21 seconds
TestRunner_smp-tester         PASS      10.40 seconds
TestRunner_userchan-tester    PASS      7.64 seconds
IncrementalBuild              PASS      38.50 seconds

Details
##############################
Test: SubjectPrefix - FAIL
Desc: Check subject contains "Bluetooth" prefix
Output:
"Bluetooth: " prefix is not specified in the subject


---
Regards,
Linux Bluetooth

Re: BUG: KASAN: null-ptr-deref in _raw_spin_lock_bh+0x4c/0xc0

[Eric Dumazet]

On Thu, Jan 19, 2023 at 2:41 AM Sungwoo Kim <iam@sung-woo.kim> wrote:
>
> Write of size 4 at addr 0000000000000098 by task kworker/u3:0/76
>
> CPU: 0 PID: 76 Comm: kworker/u3:0 Not tainted 6.1.0-rc2 #129
> Hardware name: QEMU Standard PC (i440FX + PIIX, 1996), BIOS 1.13.0-1ubuntu1.1 04/01/2014
> Workqueue: hci0 hci_rx_work
> Call Trace:
>  <TASK>
>  dump_stack_lvl+0x7b/0xb3
>  print_report+0xed/0x200
>  ? __virt_addr_valid+0x5c/0x240
>  ? kasan_addr_to_slab+0xd/0xa0
>  ? _raw_spin_lock_bh+0x4c/0xc0
>  kasan_report+0xd3/0x100
>  ? _raw_spin_lock_bh+0x4c/0xc0
>  kasan_check_range+0x2d3/0x310
>  __kasan_check_write+0x14/0x20
>  _raw_spin_lock_bh+0x4c/0xc0
>  lock_sock_nested+0x3f/0x160
>  ? queue_work_on+0x90/0xd0
>  l2cap_sock_set_shutdown_cb+0x3d/0x60
>  l2cap_disconnect_req+0x1e3/0x2e0
>  l2cap_bredr_sig_cmd+0x3d2/0x5ec0
>  ? vprintk_emit+0x29b/0x4d0
>  ? vprintk_default+0x2b/0x30
>  ? vprintk+0xdc/0x100
>  ? _printk+0x67/0x85
>  ? bt_err+0x7f/0xc0
>  ? bt_err+0x9a/0xc0
>  l2cap_recv_frame+0x7bc/0x4e10
>  ? _printk+0x67/0x85
>  ? bt_err+0x7f/0xc0
>  ? __wake_up_klogd+0xc4/0xf0
>  l2cap_recv_acldata+0x327/0x650
>  ? hci_conn_enter_active_mode+0x1b7/0x1f0
>  hci_rx_work+0x6b7/0x7c0
>  process_one_work+0x461/0xaf0
>  worker_thread+0x5f8/0xba0
>  kthread+0x1c5/0x200
>  ? process_one_work+0xaf0/0xaf0
>  ? kthread_blkcg+0x90/0x90
>  ret_from_fork+0x1f/0x30
>  </TASK>

If you plan sending more stack traces like that in the future, always
run scripts/decode_stacktrace.sh
so that the public report includes symbols.

Thanks.

Re: [PATCH] L2CAP: Fix null-ptr-deref in l2cap_sock_set_shutdown_cb

[Eric Dumazet]

On Thu, Jan 19, 2023 at 2:35 AM Sungwoo Kim <iam@sung-woo.kim> wrote:
>
> The L2CAP socket shutdown invokes l2cap_sock_destruct without a lock
> on conn->chan_lock, assigning NULL to chan->data *just before*
> the l2cap_disconnect_req thread that accesses to chan->data.

This is racy then ?

> This patch prevent it by adding a null check for a workaround, instead
> of fixing a lock.

This would at least require some barriers I think.

What about other _cb helpers also reading/using chan->data ?

>
> This bug is found by FuzzBT, a modified Syzkaller by Sungwoo Kim(me).
> Ruoyu Wu(wuruoyu@me.com) and Hui Peng(benquike@gmail.com) has helped
> the FuzzBT project.
>
> Signed-off-by: Sungwoo Kim <iam@sung-woo.kim>

I would also add

Fixes: 1bff51ea59a9 ("Bluetooth: fix use-after-free error in
lock_sock_nested()")

> ---
>  net/bluetooth/l2cap_sock.c | 8 +++++---
>  1 file changed, 5 insertions(+), 3 deletions(-)
>
> diff --git a/net/bluetooth/l2cap_sock.c b/net/bluetooth/l2cap_sock.c
> index ca8f07f35..350c7afdf 100644
> --- a/net/bluetooth/l2cap_sock.c
> +++ b/net/bluetooth/l2cap_sock.c
> @@ -1681,9 +1681,11 @@ static void l2cap_sock_set_shutdown_cb(struct l2cap_chan *chan)
>  {
>         struct sock *sk = chan->data;
>

Other similar fixes simply do:

     if (!sk)
          return;

I would chose to use the same coding style in net/bluetooth/l2cap_sock.c

> -       lock_sock(sk);
> -       sk->sk_shutdown = SHUTDOWN_MASK;
> -       release_sock(sk);
> +       if (!sk) {
> +               lock_sock(sk);
> +               sk->sk_shutdown = SHUTDOWN_MASK;
> +               release_sock(sk);
> +       }
>  }
>
>  static long l2cap_sock_get_sndtimeo_cb(struct l2cap_chan *chan)
> --
> 2.25.1
>

Re: [PATCH] L2CAP: Fix null-ptr-deref in l2cap_sock_set_shutdown_cb

[Luiz Augusto von Dentz]

Hi Kim, Eric,

On Wed, Jan 18, 2023 at 8:16 PM Eric Dumazet <edumazet@google.com> wrote:
>
> On Thu, Jan 19, 2023 at 2:35 AM Sungwoo Kim <iam@sung-woo.kim> wrote:
> >
> > The L2CAP socket shutdown invokes l2cap_sock_destruct without a lock
> > on conn->chan_lock, assigning NULL to chan->data *just before*
> > the l2cap_disconnect_req thread that accesses to chan->data.
>
> This is racy then ?
>
> > This patch prevent it by adding a null check for a workaround, instead
> > of fixing a lock.
>
> This would at least require some barriers I think.
>
> What about other _cb helpers also reading/using chan->data ?

Perhaps it would be a good idea to include the stack backtrace so we
can better understand it, at some point we might need to refactor or
locks to avoid circular dependencies.

> >
> > This bug is found by FuzzBT, a modified Syzkaller by Sungwoo Kim(me).
> > Ruoyu Wu(wuruoyu@me.com) and Hui Peng(benquike@gmail.com) has helped
> > the FuzzBT project.
> >
> > Signed-off-by: Sungwoo Kim <iam@sung-woo.kim>
>
> I would also add
>
> Fixes: 1bff51ea59a9 ("Bluetooth: fix use-after-free error in
> lock_sock_nested()")

+1

> > ---
> >  net/bluetooth/l2cap_sock.c | 8 +++++---
> >  1 file changed, 5 insertions(+), 3 deletions(-)
> >
> > diff --git a/net/bluetooth/l2cap_sock.c b/net/bluetooth/l2cap_sock.c
> > index ca8f07f35..350c7afdf 100644
> > --- a/net/bluetooth/l2cap_sock.c
> > +++ b/net/bluetooth/l2cap_sock.c
> > @@ -1681,9 +1681,11 @@ static void l2cap_sock_set_shutdown_cb(struct l2cap_chan *chan)
> >  {
> >         struct sock *sk = chan->data;
> >
>
> Other similar fixes simply do:
>
>      if (!sk)
>           return;
>
> I would chose to use the same coding style in net/bluetooth/l2cap_sock.c

Yep, at least l2cap_sock_close_cb and l2cap_sock_shutdown do that already.

>
> > -       lock_sock(sk);
> > -       sk->sk_shutdown = SHUTDOWN_MASK;
> > -       release_sock(sk);
> > +       if (!sk) {
> > +               lock_sock(sk);
> > +               sk->sk_shutdown = SHUTDOWN_MASK;
> > +               release_sock(sk);
> > +       }
> >  }
> >
> >  static long l2cap_sock_get_sndtimeo_cb(struct l2cap_chan *chan)
> > --
> > 2.25.1
> >



-- 
Luiz Augusto von Dentz