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([email protected]) and Hui Peng([email protected]) has helped
the FuzzBT project.
Signed-off-by: Sungwoo Kim <[email protected]>
---
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
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>
Fix a critical typo on the prev patch - Sorry!
Signed-off-by: Sungwoo Kim <[email protected]>
---
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
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
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
On Thu, Jan 19, 2023 at 2:41 AM Sungwoo Kim <[email protected]> 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.
On Thu, Jan 19, 2023 at 2:35 AM Sungwoo Kim <[email protected]> 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([email protected]) and Hui Peng([email protected]) has helped
> the FuzzBT project.
>
> Signed-off-by: Sungwoo Kim <[email protected]>
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
>
Hi Kim, Eric,
On Wed, Jan 18, 2023 at 8:16 PM Eric Dumazet <[email protected]> wrote:
>
> On Thu, Jan 19, 2023 at 2:35 AM Sungwoo Kim <[email protected]> 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([email protected]) and Hui Peng([email protected]) has helped
> > the FuzzBT project.
> >
> > Signed-off-by: Sungwoo Kim <[email protected]>
>
> 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