net/bluetooth/hci_core.c | 3 ++- net/bluetooth/l2cap_core.c | 2 -- 2 files changed, 2 insertions(+), 3 deletions(-)
[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);
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;
--
2.43.0Hi 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
>
--
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? -- Luiz Augusto von Dentz
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 -- Aleksandr > > -- > Luiz Augusto von Dentz >
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,
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
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;
> }
--
Aleksandr
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=127c12a9980000
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
--
Luiz Augusto von Dentz
On Fri, 20 Sep 2024 11:07:05 -0400, Luiz Augusto von Dentz wrote:
> 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.
I haven't thought of any better way yet. I understand what you mean, holding
hci_dev_lock for a long time.
At present, it is not impossible to try some strong drugs.
> >
> > >
> > > 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.
I think 7B064edae38d is used to solve another issue.
> >
> > 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?
Sorry for late.
>
> > >
> > > l2cap_connect(conn, cmd, data, L2CAP_CONN_RSP);
> > > return 0;
> > > --
> > > 2.43.0
> > >
> >
> >
> > --
> > Luiz Augusto von Dentz
BR,
Edward
© 2016 - 2024 Red Hat, Inc.