| Message ID | 20230119013405.3870506-1-iam@sung-woo.kim (mailing list archive) |
|---|---|
| State | Superseded |
| Headers | show |
| Series | L2CAP: Fix null-ptr-deref in l2cap_sock_set_shutdown_cb | expand |
| Context | Check | Description |
|---|---|---|
| tedd_an/pre-ci_am | success | Success |
| tedd_an/CheckPatch | success | CheckPatch PASS |
| tedd_an/GitLint | success | Gitlint PASS |
| tedd_an/SubjectPrefix | fail | "Bluetooth: " prefix is not specified in the subject |
| tedd_an/BuildKernel | success | BuildKernel PASS |
| tedd_an/CheckAllWarning | success | CheckAllWarning PASS |
| tedd_an/CheckSparse | success | CheckSparse PASS |
| tedd_an/CheckSmatch | success | CheckSparse PASS |
| tedd_an/BuildKernel32 | success | BuildKernel32 PASS |
| tedd_an/TestRunnerSetup | success | TestRunnerSetup PASS |
| tedd_an/TestRunner_l2cap-tester | success | TestRunner PASS |
| tedd_an/TestRunner_iso-tester | success | TestRunner PASS |
| tedd_an/TestRunner_bnep-tester | success | TestRunner PASS |
| tedd_an/TestRunner_mgmt-tester | success | TestRunner PASS |
| tedd_an/TestRunner_rfcomm-tester | success | TestRunner PASS |
| tedd_an/TestRunner_sco-tester | success | TestRunner PASS |
| tedd_an/TestRunner_ioctl-tester | success | TestRunner PASS |
| tedd_an/TestRunner_mesh-tester | success | TestRunner PASS |
| tedd_an/TestRunner_smp-tester | success | TestRunner PASS |
| tedd_an/TestRunner_userchan-tester | success | TestRunner PASS |
| tedd_an/IncrementalBuild | success | Incremental Build PASS |
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>
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 <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.
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 >
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 > >
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)
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(-)