| Message ID | tencent_7697B4E9DEEC52CEB478626D182DE96CB00A@qq.com |
|---|---|
| State | New |
| Headers | show |
| Series | Bluetooth/l2cap: Fix uaf in l2cap_connect | expand |
Hi Edward, On Sun, Sep 8, 2024 at 3:22 AM Edward Adam Davis <eadavis@qq.com> wrote: > > [Syzbot reported] > BUG: KASAN: slab-use-after-free in l2cap_connect.constprop.0+0x10d8/0x1270 net/bluetooth/l2cap_core.c:3949 > Read of size 8 at addr ffff8880241e9800 by task kworker/u9:0/54 > > CPU: 0 UID: 0 PID: 54 Comm: kworker/u9:0 Not tainted 6.11.0-rc6-syzkaller-00268-g788220eee30d #0 > Hardware name: Google Google Compute Engine/Google Compute Engine, BIOS Google 08/06/2024 > Workqueue: hci2 hci_rx_work > Call Trace: > <TASK> > __dump_stack lib/dump_stack.c:93 [inline] > dump_stack_lvl+0x116/0x1f0 lib/dump_stack.c:119 > print_address_description mm/kasan/report.c:377 [inline] > print_report+0xc3/0x620 mm/kasan/report.c:488 > kasan_report+0xd9/0x110 mm/kasan/report.c:601 > l2cap_connect.constprop.0+0x10d8/0x1270 net/bluetooth/l2cap_core.c:3949 > l2cap_connect_req net/bluetooth/l2cap_core.c:4080 [inline] > l2cap_bredr_sig_cmd net/bluetooth/l2cap_core.c:4772 [inline] > l2cap_sig_channel net/bluetooth/l2cap_core.c:5543 [inline] > l2cap_recv_frame+0xf0b/0x8eb0 net/bluetooth/l2cap_core.c:6825 > l2cap_recv_acldata+0x9b4/0xb70 net/bluetooth/l2cap_core.c:7514 > hci_acldata_packet net/bluetooth/hci_core.c:3791 [inline] > hci_rx_work+0xaab/0x1610 net/bluetooth/hci_core.c:4028 > process_one_work+0x9c5/0x1b40 kernel/workqueue.c:3231 > process_scheduled_works kernel/workqueue.c:3312 [inline] > worker_thread+0x6c8/0xed0 kernel/workqueue.c:3389 > kthread+0x2c1/0x3a0 kernel/kthread.c:389 > ret_from_fork+0x45/0x80 arch/x86/kernel/process.c:147 > ret_from_fork_asm+0x1a/0x30 arch/x86/entry/entry_64.S:244 > ... > > Freed by task 5245: > kasan_save_stack+0x33/0x60 mm/kasan/common.c:47 > kasan_save_track+0x14/0x30 mm/kasan/common.c:68 > kasan_save_free_info+0x3b/0x60 mm/kasan/generic.c:579 > poison_slab_object+0xf7/0x160 mm/kasan/common.c:240 > __kasan_slab_free+0x32/0x50 mm/kasan/common.c:256 > kasan_slab_free include/linux/kasan.h:184 [inline] > slab_free_hook mm/slub.c:2256 [inline] > slab_free mm/slub.c:4477 [inline] > kfree+0x12a/0x3b0 mm/slub.c:4598 > l2cap_conn_free net/bluetooth/l2cap_core.c:1810 [inline] > kref_put include/linux/kref.h:65 [inline] > l2cap_conn_put net/bluetooth/l2cap_core.c:1822 [inline] > l2cap_conn_del+0x59d/0x730 net/bluetooth/l2cap_core.c:1802 > l2cap_connect_cfm+0x9e6/0xf80 net/bluetooth/l2cap_core.c:7241 > hci_connect_cfm include/net/bluetooth/hci_core.h:1960 [inline] > hci_conn_failed+0x1c3/0x370 net/bluetooth/hci_conn.c:1265 > hci_abort_conn_sync+0x75a/0xb50 net/bluetooth/hci_sync.c:5583 > abort_conn_sync+0x197/0x360 net/bluetooth/hci_conn.c:2917 > hci_cmd_sync_work+0x1a4/0x410 net/bluetooth/hci_sync.c:328 > process_one_work+0x9c5/0x1b40 kernel/workqueue.c:3231 > process_scheduled_works kernel/workqueue.c:3312 [inline] > worker_thread+0x6c8/0xed0 kernel/workqueue.c:3389 > kthread+0x2c1/0x3a0 kernel/kthread.c:389 > ret_from_fork+0x45/0x80 arch/x86/kernel/process.c:147 > ret_from_fork_asm+0x1a/0x30 arch/x86/entry/entry_64.S:244 > > [Analysis] > There was a data race when accessing conn in hci_rx_work and hci_cmd_sync_work. > This is because the hci dev lock was prematurely exited when executing > hci_acldata_macket() in hci_rx_work, which resulted in it being released > by hci_cmd_sync_work when accessing conn outside the lock. > > Reported-and-tested-by: syzbot+c12e2f941af1feb5632c@syzkaller.appspotmail.com > Closes: https://syzkaller.appspot.com/bug?extid=c12e2f941af1feb5632c > Signed-off-by: Edward Adam Davis <eadavis@qq.com> > --- > net/bluetooth/hci_core.c | 3 ++- > net/bluetooth/l2cap_core.c | 2 -- > 2 files changed, 2 insertions(+), 3 deletions(-) > > diff --git a/net/bluetooth/hci_core.c b/net/bluetooth/hci_core.c > index f25a21f532aa..4f7b45bb863f 100644 > --- a/net/bluetooth/hci_core.c > +++ b/net/bluetooth/hci_core.c > @@ -3776,18 +3776,19 @@ static void hci_acldata_packet(struct hci_dev *hdev, struct sk_buff *skb) > > hci_dev_lock(hdev); > conn = hci_conn_hash_lookup_handle(hdev, handle); > - hci_dev_unlock(hdev); > > if (conn) { > hci_conn_enter_active_mode(conn, BT_POWER_FORCE_ACTIVE_OFF); > > /* Send to upper protocol */ > l2cap_recv_acldata(conn, skb, flags); > + hci_dev_unlock(hdev); > return; > } else { > bt_dev_err(hdev, "ACL packet for unknown connection handle %d", > handle); > } > + hci_dev_unlock(hdev); This is sort of risky, we shouldn't be calling this deep into the stack with hci_dev_lock held. > > kfree_skb(skb); > } > diff --git a/net/bluetooth/l2cap_core.c b/net/bluetooth/l2cap_core.c > index 9988ba382b68..b948b0a3b2f2 100644 > --- a/net/bluetooth/l2cap_core.c > +++ b/net/bluetooth/l2cap_core.c > @@ -4072,10 +4072,8 @@ static int l2cap_connect_req(struct l2cap_conn *conn, > if (cmd_len < sizeof(struct l2cap_conn_req)) > return -EPROTO; > > - hci_dev_lock(hdev); > if (hci_dev_test_flag(hdev, HCI_MGMT)) > mgmt_device_connected(hdev, hcon, NULL, 0); > - hci_dev_unlock(hdev); So this might explain why things gets freed while processing the request, we are locking to call mgmt_device_connected which I suspect is no longer needed ever since: commit db11223571d489d1aab575a4ac4b7352d2d54e2f Author: Luiz Augusto von Dentz <luiz.von.dentz@intel.com> Date: Tue Oct 25 14:12:58 2022 -0700 Bluetooth: btusb: Default CONFIG_BT_HCIBTUSB_POLL_SYNC=y poll_sync has been proven to fix races of USB data and event endpoints so this enables it by default. Signed-off-by: Luiz Augusto von Dentz <luiz.von.dentz@intel.com> Tested-by: Tedd Ho-Jeong An <tedd.an@intel.com> Anyway syzbot don't use btusb so I think this might be due some command pending that the emulator is not responding and instead sending data, and then there is the issue that 7b064edae38d ("Bluetooth: Fix authentication if acl data comes before remote feature evt") attempted to fix which I think it actually made it worse by moving the call to mgmt_device_connected into l2cap_core.c it sort move the problem but didn't fix the actual problem. Maybe something like the following would be a better approach: https://gist.github.com/Vudentz/121a15fa4391b2b1f6c7e8d420a6846e > > l2cap_connect(conn, cmd, data, L2CAP_CONN_RSP); > return 0; > -- > 2.43.0 >
Hi Edward, On Tue, Sep 10, 2024 at 4:56 PM Luiz Augusto von Dentz <luiz.dentz@gmail.com> wrote: > > Hi Edward, > > On Sun, Sep 8, 2024 at 3:22 AM Edward Adam Davis <eadavis@qq.com> wrote: > > > > [Syzbot reported] > > BUG: KASAN: slab-use-after-free in l2cap_connect.constprop.0+0x10d8/0x1270 net/bluetooth/l2cap_core.c:3949 > > Read of size 8 at addr ffff8880241e9800 by task kworker/u9:0/54 > > > > CPU: 0 UID: 0 PID: 54 Comm: kworker/u9:0 Not tainted 6.11.0-rc6-syzkaller-00268-g788220eee30d #0 > > Hardware name: Google Google Compute Engine/Google Compute Engine, BIOS Google 08/06/2024 > > Workqueue: hci2 hci_rx_work > > Call Trace: > > <TASK> > > __dump_stack lib/dump_stack.c:93 [inline] > > dump_stack_lvl+0x116/0x1f0 lib/dump_stack.c:119 > > print_address_description mm/kasan/report.c:377 [inline] > > print_report+0xc3/0x620 mm/kasan/report.c:488 > > kasan_report+0xd9/0x110 mm/kasan/report.c:601 > > l2cap_connect.constprop.0+0x10d8/0x1270 net/bluetooth/l2cap_core.c:3949 > > l2cap_connect_req net/bluetooth/l2cap_core.c:4080 [inline] > > l2cap_bredr_sig_cmd net/bluetooth/l2cap_core.c:4772 [inline] > > l2cap_sig_channel net/bluetooth/l2cap_core.c:5543 [inline] > > l2cap_recv_frame+0xf0b/0x8eb0 net/bluetooth/l2cap_core.c:6825 > > l2cap_recv_acldata+0x9b4/0xb70 net/bluetooth/l2cap_core.c:7514 > > hci_acldata_packet net/bluetooth/hci_core.c:3791 [inline] > > hci_rx_work+0xaab/0x1610 net/bluetooth/hci_core.c:4028 > > process_one_work+0x9c5/0x1b40 kernel/workqueue.c:3231 > > process_scheduled_works kernel/workqueue.c:3312 [inline] > > worker_thread+0x6c8/0xed0 kernel/workqueue.c:3389 > > kthread+0x2c1/0x3a0 kernel/kthread.c:389 > > ret_from_fork+0x45/0x80 arch/x86/kernel/process.c:147 > > ret_from_fork_asm+0x1a/0x30 arch/x86/entry/entry_64.S:244 > > ... > > > > Freed by task 5245: > > kasan_save_stack+0x33/0x60 mm/kasan/common.c:47 > > kasan_save_track+0x14/0x30 mm/kasan/common.c:68 > > kasan_save_free_info+0x3b/0x60 mm/kasan/generic.c:579 > > poison_slab_object+0xf7/0x160 mm/kasan/common.c:240 > > __kasan_slab_free+0x32/0x50 mm/kasan/common.c:256 > > kasan_slab_free include/linux/kasan.h:184 [inline] > > slab_free_hook mm/slub.c:2256 [inline] > > slab_free mm/slub.c:4477 [inline] > > kfree+0x12a/0x3b0 mm/slub.c:4598 > > l2cap_conn_free net/bluetooth/l2cap_core.c:1810 [inline] > > kref_put include/linux/kref.h:65 [inline] > > l2cap_conn_put net/bluetooth/l2cap_core.c:1822 [inline] > > l2cap_conn_del+0x59d/0x730 net/bluetooth/l2cap_core.c:1802 > > l2cap_connect_cfm+0x9e6/0xf80 net/bluetooth/l2cap_core.c:7241 > > hci_connect_cfm include/net/bluetooth/hci_core.h:1960 [inline] > > hci_conn_failed+0x1c3/0x370 net/bluetooth/hci_conn.c:1265 > > hci_abort_conn_sync+0x75a/0xb50 net/bluetooth/hci_sync.c:5583 > > abort_conn_sync+0x197/0x360 net/bluetooth/hci_conn.c:2917 > > hci_cmd_sync_work+0x1a4/0x410 net/bluetooth/hci_sync.c:328 > > process_one_work+0x9c5/0x1b40 kernel/workqueue.c:3231 > > process_scheduled_works kernel/workqueue.c:3312 [inline] > > worker_thread+0x6c8/0xed0 kernel/workqueue.c:3389 > > kthread+0x2c1/0x3a0 kernel/kthread.c:389 > > ret_from_fork+0x45/0x80 arch/x86/kernel/process.c:147 > > ret_from_fork_asm+0x1a/0x30 arch/x86/entry/entry_64.S:244 > > > > [Analysis] > > There was a data race when accessing conn in hci_rx_work and hci_cmd_sync_work. > > This is because the hci dev lock was prematurely exited when executing > > hci_acldata_macket() in hci_rx_work, which resulted in it being released > > by hci_cmd_sync_work when accessing conn outside the lock. > > > > Reported-and-tested-by: syzbot+c12e2f941af1feb5632c@syzkaller.appspotmail.com > > Closes: https://syzkaller.appspot.com/bug?extid=c12e2f941af1feb5632c > > Signed-off-by: Edward Adam Davis <eadavis@qq.com> > > --- > > net/bluetooth/hci_core.c | 3 ++- > > net/bluetooth/l2cap_core.c | 2 -- > > 2 files changed, 2 insertions(+), 3 deletions(-) > > > > diff --git a/net/bluetooth/hci_core.c b/net/bluetooth/hci_core.c > > index f25a21f532aa..4f7b45bb863f 100644 > > --- a/net/bluetooth/hci_core.c > > +++ b/net/bluetooth/hci_core.c > > @@ -3776,18 +3776,19 @@ static void hci_acldata_packet(struct hci_dev *hdev, struct sk_buff *skb) > > > > hci_dev_lock(hdev); > > conn = hci_conn_hash_lookup_handle(hdev, handle); > > - hci_dev_unlock(hdev); > > > > if (conn) { > > hci_conn_enter_active_mode(conn, BT_POWER_FORCE_ACTIVE_OFF); > > > > /* Send to upper protocol */ > > l2cap_recv_acldata(conn, skb, flags); > > + hci_dev_unlock(hdev); > > return; > > } else { > > bt_dev_err(hdev, "ACL packet for unknown connection handle %d", > > handle); > > } > > + hci_dev_unlock(hdev); > > This is sort of risky, we shouldn't be calling this deep into the > stack with hci_dev_lock held. > > > > > kfree_skb(skb); > > } > > diff --git a/net/bluetooth/l2cap_core.c b/net/bluetooth/l2cap_core.c > > index 9988ba382b68..b948b0a3b2f2 100644 > > --- a/net/bluetooth/l2cap_core.c > > +++ b/net/bluetooth/l2cap_core.c > > @@ -4072,10 +4072,8 @@ static int l2cap_connect_req(struct l2cap_conn *conn, > > if (cmd_len < sizeof(struct l2cap_conn_req)) > > return -EPROTO; > > > > - hci_dev_lock(hdev); > > if (hci_dev_test_flag(hdev, HCI_MGMT)) > > mgmt_device_connected(hdev, hcon, NULL, 0); > > - hci_dev_unlock(hdev); > > So this might explain why things gets freed while processing the > request, we are locking to call mgmt_device_connected which I suspect > is no longer needed ever since: > > commit db11223571d489d1aab575a4ac4b7352d2d54e2f > Author: Luiz Augusto von Dentz <luiz.von.dentz@intel.com> > Date: Tue Oct 25 14:12:58 2022 -0700 > > Bluetooth: btusb: Default CONFIG_BT_HCIBTUSB_POLL_SYNC=y > > poll_sync has been proven to fix races of USB data and event endpoints > so this enables it by default. > > Signed-off-by: Luiz Augusto von Dentz <luiz.von.dentz@intel.com> > Tested-by: Tedd Ho-Jeong An <tedd.an@intel.com> > > Anyway syzbot don't use btusb so I think this might be due some > command pending that the emulator is not responding and instead > sending data, and then there is the issue that 7b064edae38d > ("Bluetooth: Fix authentication if acl data comes before remote > feature evt") attempted to fix which I think it actually made it worse > by moving the call to mgmt_device_connected into l2cap_core.c it sort > move the problem but didn't fix the actual problem. > > Maybe something like the following would be a better approach: > > https://gist.github.com/Vudentz/121a15fa4391b2b1f6c7e8d420a6846e Any comments? Are you still planning to work on this? > > > > l2cap_connect(conn, cmd, data, L2CAP_CONN_RSP); > > return 0; > > -- > > 2.43.0 > > > > > -- > Luiz Augusto von Dentz
On Tue, Sep 10, 2024 at 4:56 PM Luiz Augusto von Dentz <luiz.dentz@gmail.com> wrote: > > Maybe something like the following would be a better approach: > > https://gist.github.com/Vudentz/121a15fa4391b2b1f6c7e8d420a6846e If your idea is not bad, boy, feel free to win Tested-by from syzbot with it.
Hi Hillf, On Sat, Sep 21, 2024 at 6:56 AM Hillf Danton <hdanton@sina.com> wrote: > > On Tue, Sep 10, 2024 at 4:56 PM Luiz Augusto von Dentz <luiz.dentz@gmail.com> wrote: > > > > Maybe something like the following would be a better approach: > > > > https://gist.github.com/Vudentz/121a15fa4391b2b1f6c7e8d420a6846e > > If your idea is not bad, boy, feel free to win Tested-by from syzbot with it. Is there a way to quickly check a patch with syzbot?
On Mon, Sep 23, 2024 at 4:33 PM Luiz Augusto von Dentz <luiz.dentz@gmail.com> wrote: > > Hi Hillf, > > On Sat, Sep 21, 2024 at 6:56 AM Hillf Danton <hdanton@sina.com> wrote: > > > > On Tue, Sep 10, 2024 at 4:56 PM Luiz Augusto von Dentz <luiz.dentz@gmail.com> wrote: > > > > > > Maybe something like the following would be a better approach: > > > > > > https://gist.github.com/Vudentz/121a15fa4391b2b1f6c7e8d420a6846e > > > > If your idea is not bad, boy, feel free to win Tested-by from syzbot with it. > > Is there a way to quickly check a patch with syzbot? You can send a `#syz test` command in a reply to syzbot and attach the patch-to-test to the email message. See https://github.com/google/syzkaller/blob/master/docs/syzbot.md#testing-patches
Hi Aleksandr, On Mon, Sep 23, 2024 at 10:37 AM Aleksandr Nogikh <nogikh@google.com> wrote: > > On Mon, Sep 23, 2024 at 4:33 PM Luiz Augusto von Dentz > <luiz.dentz@gmail.com> wrote: > > > > Hi Hillf, > > > > On Sat, Sep 21, 2024 at 6:56 AM Hillf Danton <hdanton@sina.com> wrote: > > > > > > On Tue, Sep 10, 2024 at 4:56 PM Luiz Augusto von Dentz <luiz.dentz@gmail.com> wrote: > > > > > > > > Maybe something like the following would be a better approach: > > > > > > > > https://gist.github.com/Vudentz/121a15fa4391b2b1f6c7e8d420a6846e > > > > > > If your idea is not bad, boy, feel free to win Tested-by from syzbot with it. > > > > Is there a way to quickly check a patch with syzbot? > > You can send a `#syz test` command in a reply to syzbot and attach the > patch-to-test to the email message. > > See https://github.com/google/syzkaller/blob/master/docs/syzbot.md#testing-patches Thanks, lets see if this works: #syz test diff --git a/net/bluetooth/hci_core.c b/net/bluetooth/hci_core.c index d6976db02c06..b2f8f9c5b610 100644 --- a/net/bluetooth/hci_core.c +++ b/net/bluetooth/hci_core.c @@ -3782,6 +3782,8 @@ static void hci_acldata_packet(struct hci_dev *hdev, struct sk_buff *skb) hci_dev_lock(hdev); conn = hci_conn_hash_lookup_handle(hdev, handle); + if (conn && hci_dev_test_flag(hdev, HCI_MGMT)) + mgmt_device_connected(hdev, conn, NULL, 0); hci_dev_unlock(hdev); if (conn) { diff --git a/net/bluetooth/hci_event.c b/net/bluetooth/hci_event.c index 1c82dcdf6e8f..b87c0f1dab9e 100644 --- a/net/bluetooth/hci_event.c +++ b/net/bluetooth/hci_event.c @@ -3706,7 +3706,7 @@ static void hci_remote_features_evt(struct hci_dev *hdev, void *data, goto unlock; } - if (!ev->status && !test_bit(HCI_CONN_MGMT_CONNECTED, &conn->flags)) { + if (!ev->status) { struct hci_cp_remote_name_req cp; memset(&cp, 0, sizeof(cp)); bacpy(&cp.bdaddr, &conn->dst); diff --git a/net/bluetooth/l2cap_core.c b/net/bluetooth/l2cap_core.c index 9988ba382b68..6544c1ed7143 100644 --- a/net/bluetooth/l2cap_core.c +++ b/net/bluetooth/l2cap_core.c @@ -4066,17 +4066,9 @@ static void l2cap_connect(struct l2cap_conn *conn, struct l2cap_cmd_hdr *cmd, static int l2cap_connect_req(struct l2cap_conn *conn, struct l2cap_cmd_hdr *cmd, u16 cmd_len, u8 *data) { - struct hci_dev *hdev = conn->hcon->hdev; - struct hci_conn *hcon = conn->hcon; - if (cmd_len < sizeof(struct l2cap_conn_req)) return -EPROTO; - hci_dev_lock(hdev); - if (hci_dev_test_flag(hdev, HCI_MGMT)) - mgmt_device_connected(hdev, hcon, NULL, 0); - hci_dev_unlock(hdev); - l2cap_connect(conn, cmd, data, L2CAP_CONN_RSP); return 0; }
Hi Luiz, On Mon, Sep 23, 2024 at 5:20 PM Luiz Augusto von Dentz <luiz.dentz@gmail.com> wrote: > > Hi Aleksandr, > > On Mon, Sep 23, 2024 at 10:37 AM Aleksandr Nogikh <nogikh@google.com> wrote: > > > > On Mon, Sep 23, 2024 at 4:33 PM Luiz Augusto von Dentz > > <luiz.dentz@gmail.com> wrote: > > > > > > Hi Hillf, > > > > > > On Sat, Sep 21, 2024 at 6:56 AM Hillf Danton <hdanton@sina.com> wrote: > > > > > > > > On Tue, Sep 10, 2024 at 4:56 PM Luiz Augusto von Dentz <luiz.dentz@gmail.com> wrote: > > > > > > > > > > Maybe something like the following would be a better approach: > > > > > > > > > > https://gist.github.com/Vudentz/121a15fa4391b2b1f6c7e8d420a6846e > > > > > > > > If your idea is not bad, boy, feel free to win Tested-by from syzbot with it. > > > > > > Is there a way to quickly check a patch with syzbot? > > > > You can send a `#syz test` command in a reply to syzbot and attach the > > patch-to-test to the email message. > > > > See https://github.com/google/syzkaller/blob/master/docs/syzbot.md#testing-patches > > Thanks, lets see if this works: > > #syz test > > diff --git a/net/bluetooth/hci_core.c b/net/bluetooth/hci_core.c > index d6976db02c06..b2f8f9c5b610 100644 > --- a/net/bluetooth/hci_core.c > +++ b/net/bluetooth/hci_core.c > @@ -3782,6 +3782,8 @@ static void hci_acldata_packet(struct hci_dev > *hdev, struct sk_buff *skb) > > hci_dev_lock(hdev); > conn = hci_conn_hash_lookup_handle(hdev, handle); > + if (conn && hci_dev_test_flag(hdev, HCI_MGMT)) > + mgmt_device_connected(hdev, conn, NULL, 0); > hci_dev_unlock(hdev); > ^^ Patch parsing will fail here because it expects to see the git diff output as is -- i.e. if some line only consisted of a single whitespace (= it was an empty line and it did not change), it must remain so. Sometimes these whitespaces get lost during copy-pasting and it confuses syzbot. > if (conn) { > diff --git a/net/bluetooth/hci_event.c b/net/bluetooth/hci_event.c > index 1c82dcdf6e8f..b87c0f1dab9e 100644 > --- a/net/bluetooth/hci_event.c > +++ b/net/bluetooth/hci_event.c > @@ -3706,7 +3706,7 @@ static void hci_remote_features_evt(struct > hci_dev *hdev, void *data, > goto unlock; > } > > - if (!ev->status && !test_bit(HCI_CONN_MGMT_CONNECTED, &conn->flags)) { > + if (!ev->status) { > struct hci_cp_remote_name_req cp; > memset(&cp, 0, sizeof(cp)); > bacpy(&cp.bdaddr, &conn->dst); > diff --git a/net/bluetooth/l2cap_core.c b/net/bluetooth/l2cap_core.c > index 9988ba382b68..6544c1ed7143 100644 > --- a/net/bluetooth/l2cap_core.c > +++ b/net/bluetooth/l2cap_core.c > @@ -4066,17 +4066,9 @@ static void l2cap_connect(struct l2cap_conn > *conn, struct l2cap_cmd_hdr *cmd, > static int l2cap_connect_req(struct l2cap_conn *conn, > struct l2cap_cmd_hdr *cmd, u16 cmd_len, u8 *data) > { > - struct hci_dev *hdev = conn->hcon->hdev; > - struct hci_conn *hcon = conn->hcon; > - > if (cmd_len < sizeof(struct l2cap_conn_req)) > return -EPROTO; > > - hci_dev_lock(hdev); > - if (hci_dev_test_flag(hdev, HCI_MGMT)) > - mgmt_device_connected(hdev, hcon, NULL, 0); > - hci_dev_unlock(hdev); > - > l2cap_connect(conn, cmd, data, L2CAP_CONN_RSP); > return 0; > }
Hi, On Mon, Sep 23, 2024 at 11:20 AM Luiz Augusto von Dentz <luiz.dentz@gmail.com> wrote: > > Hi Aleksandr, > > On Mon, Sep 23, 2024 at 10:37 AM Aleksandr Nogikh <nogikh@google.com> wrote: > > > > On Mon, Sep 23, 2024 at 4:33 PM Luiz Augusto von Dentz > > <luiz.dentz@gmail.com> wrote: > > > > > > Hi Hillf, > > > > > > On Sat, Sep 21, 2024 at 6:56 AM Hillf Danton <hdanton@sina.com> wrote: > > > > > > > > On Tue, Sep 10, 2024 at 4:56 PM Luiz Augusto von Dentz <luiz.dentz@gmail.com> wrote: > > > > > > > > > > Maybe something like the following would be a better approach: > > > > > > > > > > https://gist.github.com/Vudentz/121a15fa4391b2b1f6c7e8d420a6846e > > > > > > > > If your idea is not bad, boy, feel free to win Tested-by from syzbot with it. > > > > > > Is there a way to quickly check a patch with syzbot? > > > > You can send a `#syz test` command in a reply to syzbot and attach the > > patch-to-test to the email message. > > > > See https://github.com/google/syzkaller/blob/master/docs/syzbot.md#testing-patches > Lets try again with git diff output: #syz test diff --git a/net/bluetooth/hci_core.c b/net/bluetooth/hci_core.c index d6976db02c06..b2f8f9c5b610 100644 --- a/net/bluetooth/hci_core.c +++ b/net/bluetooth/hci_core.c @@ -3782,6 +3782,8 @@ static void hci_acldata_packet(struct hci_dev *hdev, struct sk_buff *skb) hci_dev_lock(hdev); conn = hci_conn_hash_lookup_handle(hdev, handle); + if (conn && hci_dev_test_flag(hdev, HCI_MGMT)) + mgmt_device_connected(hdev, conn, NULL, 0); hci_dev_unlock(hdev); if (conn) { diff --git a/net/bluetooth/hci_event.c b/net/bluetooth/hci_event.c index 1c82dcdf6e8f..b87c0f1dab9e 100644 --- a/net/bluetooth/hci_event.c +++ b/net/bluetooth/hci_event.c @@ -3706,7 +3706,7 @@ static void hci_remote_features_evt(struct hci_dev *hdev, void *data, goto unlock; } - if (!ev->status && !test_bit(HCI_CONN_MGMT_CONNECTED, &conn->flags)) { + if (!ev->status) { struct hci_cp_remote_name_req cp; memset(&cp, 0, sizeof(cp)); bacpy(&cp.bdaddr, &conn->dst); diff --git a/net/bluetooth/l2cap_core.c b/net/bluetooth/l2cap_core.c index 9988ba382b68..6544c1ed7143 100644 --- a/net/bluetooth/l2cap_core.c +++ b/net/bluetooth/l2cap_core.c @@ -4066,17 +4066,9 @@ static void l2cap_connect(struct l2cap_conn *conn, struct l2cap_cmd_hdr *cmd, static int l2cap_connect_req(struct l2cap_conn *conn, struct l2cap_cmd_hdr *cmd, u16 cmd_len, u8 *data) { - struct hci_dev *hdev = conn->hcon->hdev; - struct hci_conn *hcon = conn->hcon; - if (cmd_len < sizeof(struct l2cap_conn_req)) return -EPROTO; - hci_dev_lock(hdev); - if (hci_dev_test_flag(hdev, HCI_MGMT)) - mgmt_device_connected(hdev, hcon, NULL, 0); - hci_dev_unlock(hdev); - l2cap_connect(conn, cmd, data, L2CAP_CONN_RSP); return 0; }
Hello, syzbot tried to test the proposed patch but the build/boot failed: failed to apply patch: checking file net/bluetooth/hci_core.c patch: **** malformed patch at line 6: diff --git a/net/bluetooth/hci_event.c b/net/bluetooth/hci_event.c Tested on: commit: de5cb0dc Merge branch 'address-masking' git tree: upstream kernel config: https://syzkaller.appspot.com/x/.config?x=57042fe37c7ee7c2 dashboard link: https://syzkaller.appspot.com/bug?extid=c12e2f941af1feb5632c compiler: patch: https://syzkaller.appspot.com/x/patch.diff?x=11f72107980000
diff --git a/net/bluetooth/hci_core.c b/net/bluetooth/hci_core.c index f25a21f532aa..4f7b45bb863f 100644 --- a/net/bluetooth/hci_core.c +++ b/net/bluetooth/hci_core.c @@ -3776,18 +3776,19 @@ static void hci_acldata_packet(struct hci_dev *hdev, struct sk_buff *skb) hci_dev_lock(hdev); conn = hci_conn_hash_lookup_handle(hdev, handle); - hci_dev_unlock(hdev); if (conn) { hci_conn_enter_active_mode(conn, BT_POWER_FORCE_ACTIVE_OFF); /* Send to upper protocol */ l2cap_recv_acldata(conn, skb, flags); + hci_dev_unlock(hdev); return; } else { bt_dev_err(hdev, "ACL packet for unknown connection handle %d", handle); } + hci_dev_unlock(hdev); kfree_skb(skb); } diff --git a/net/bluetooth/l2cap_core.c b/net/bluetooth/l2cap_core.c index 9988ba382b68..b948b0a3b2f2 100644 --- a/net/bluetooth/l2cap_core.c +++ b/net/bluetooth/l2cap_core.c @@ -4072,10 +4072,8 @@ static int l2cap_connect_req(struct l2cap_conn *conn, if (cmd_len < sizeof(struct l2cap_conn_req)) return -EPROTO; - hci_dev_lock(hdev); if (hci_dev_test_flag(hdev, HCI_MGMT)) mgmt_device_connected(hdev, hcon, NULL, 0); - hci_dev_unlock(hdev); l2cap_connect(conn, cmd, data, L2CAP_CONN_RSP); return 0;
[Syzbot reported] BUG: KASAN: slab-use-after-free in l2cap_connect.constprop.0+0x10d8/0x1270 net/bluetooth/l2cap_core.c:3949 Read of size 8 at addr ffff8880241e9800 by task kworker/u9:0/54 CPU: 0 UID: 0 PID: 54 Comm: kworker/u9:0 Not tainted 6.11.0-rc6-syzkaller-00268-g788220eee30d #0 Hardware name: Google Google Compute Engine/Google Compute Engine, BIOS Google 08/06/2024 Workqueue: hci2 hci_rx_work Call Trace: <TASK> __dump_stack lib/dump_stack.c:93 [inline] dump_stack_lvl+0x116/0x1f0 lib/dump_stack.c:119 print_address_description mm/kasan/report.c:377 [inline] print_report+0xc3/0x620 mm/kasan/report.c:488 kasan_report+0xd9/0x110 mm/kasan/report.c:601 l2cap_connect.constprop.0+0x10d8/0x1270 net/bluetooth/l2cap_core.c:3949 l2cap_connect_req net/bluetooth/l2cap_core.c:4080 [inline] l2cap_bredr_sig_cmd net/bluetooth/l2cap_core.c:4772 [inline] l2cap_sig_channel net/bluetooth/l2cap_core.c:5543 [inline] l2cap_recv_frame+0xf0b/0x8eb0 net/bluetooth/l2cap_core.c:6825 l2cap_recv_acldata+0x9b4/0xb70 net/bluetooth/l2cap_core.c:7514 hci_acldata_packet net/bluetooth/hci_core.c:3791 [inline] hci_rx_work+0xaab/0x1610 net/bluetooth/hci_core.c:4028 process_one_work+0x9c5/0x1b40 kernel/workqueue.c:3231 process_scheduled_works kernel/workqueue.c:3312 [inline] worker_thread+0x6c8/0xed0 kernel/workqueue.c:3389 kthread+0x2c1/0x3a0 kernel/kthread.c:389 ret_from_fork+0x45/0x80 arch/x86/kernel/process.c:147 ret_from_fork_asm+0x1a/0x30 arch/x86/entry/entry_64.S:244 ... Freed by task 5245: kasan_save_stack+0x33/0x60 mm/kasan/common.c:47 kasan_save_track+0x14/0x30 mm/kasan/common.c:68 kasan_save_free_info+0x3b/0x60 mm/kasan/generic.c:579 poison_slab_object+0xf7/0x160 mm/kasan/common.c:240 __kasan_slab_free+0x32/0x50 mm/kasan/common.c:256 kasan_slab_free include/linux/kasan.h:184 [inline] slab_free_hook mm/slub.c:2256 [inline] slab_free mm/slub.c:4477 [inline] kfree+0x12a/0x3b0 mm/slub.c:4598 l2cap_conn_free net/bluetooth/l2cap_core.c:1810 [inline] kref_put include/linux/kref.h:65 [inline] l2cap_conn_put net/bluetooth/l2cap_core.c:1822 [inline] l2cap_conn_del+0x59d/0x730 net/bluetooth/l2cap_core.c:1802 l2cap_connect_cfm+0x9e6/0xf80 net/bluetooth/l2cap_core.c:7241 hci_connect_cfm include/net/bluetooth/hci_core.h:1960 [inline] hci_conn_failed+0x1c3/0x370 net/bluetooth/hci_conn.c:1265 hci_abort_conn_sync+0x75a/0xb50 net/bluetooth/hci_sync.c:5583 abort_conn_sync+0x197/0x360 net/bluetooth/hci_conn.c:2917 hci_cmd_sync_work+0x1a4/0x410 net/bluetooth/hci_sync.c:328 process_one_work+0x9c5/0x1b40 kernel/workqueue.c:3231 process_scheduled_works kernel/workqueue.c:3312 [inline] worker_thread+0x6c8/0xed0 kernel/workqueue.c:3389 kthread+0x2c1/0x3a0 kernel/kthread.c:389 ret_from_fork+0x45/0x80 arch/x86/kernel/process.c:147 ret_from_fork_asm+0x1a/0x30 arch/x86/entry/entry_64.S:244 [Analysis] There was a data race when accessing conn in hci_rx_work and hci_cmd_sync_work. This is because the hci dev lock was prematurely exited when executing hci_acldata_macket() in hci_rx_work, which resulted in it being released by hci_cmd_sync_work when accessing conn outside the lock. Reported-and-tested-by: syzbot+c12e2f941af1feb5632c@syzkaller.appspotmail.com Closes: https://syzkaller.appspot.com/bug?extid=c12e2f941af1feb5632c Signed-off-by: Edward Adam Davis <eadavis@qq.com> --- net/bluetooth/hci_core.c | 3 ++- net/bluetooth/l2cap_core.c | 2 -- 2 files changed, 2 insertions(+), 3 deletions(-)