| Message ID | 20230430171847.156825-1-lrh2000@pku.edu.cn |
|---|---|
| State | Accepted |
| Commit | 29f883dcbfd0c922cb9d447036bc57208e50728c |
| Headers | show |
| Series | Bluetooth: Fix UAF in hci_conn_hash_flush again | expand |
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=744073
---Test result---
Test Summary:
CheckPatch FAIL 1.12 seconds
GitLint PASS 0.35 seconds
SubjectPrefix PASS 0.13 seconds
BuildKernel PASS 32.06 seconds
CheckAllWarning PASS 34.74 seconds
CheckSparse PASS 39.61 seconds
CheckSmatch PASS 108.27 seconds
BuildKernel32 PASS 31.25 seconds
TestRunnerSetup PASS 442.52 seconds
TestRunner_l2cap-tester PASS 16.91 seconds
TestRunner_iso-tester PASS 21.38 seconds
TestRunner_bnep-tester PASS 5.63 seconds
TestRunner_mgmt-tester PASS 116.50 seconds
TestRunner_rfcomm-tester PASS 9.06 seconds
TestRunner_sco-tester PASS 8.31 seconds
TestRunner_ioctl-tester PASS 9.75 seconds
TestRunner_mesh-tester PASS 7.21 seconds
TestRunner_smp-tester PASS 8.15 seconds
TestRunner_userchan-tester PASS 5.94 seconds
IncrementalBuild PASS 29.20 seconds
Details
##############################
Test: CheckPatch - FAIL
Desc: Run checkpatch.pl script
Output:
Bluetooth: Fix UAF in hci_conn_hash_flush again
WARNING: Reported-by: should be immediately followed by Link: with a URL to the report
#92:
Reported-by: syzbot+8bb72f86fc823817bc5d@syzkaller.appspotmail.com
Closes: https://syzkaller.appspot.com/bug?extid=8bb72f86fc823817bc5d
WARNING: Unknown link reference 'Closes:', use 'Link:' instead
#93:
Closes: https://syzkaller.appspot.com/bug?extid=8bb72f86fc823817bc5d
CHECK: Alignment should match open parenthesis
#163: FILE: net/bluetooth/hci_conn.c:2473:
+ while ((conn = list_first_entry_or_null(head,
+ struct hci_conn, list)) != NULL) {
total: 0 errors, 2 warnings, 1 checks, 57 lines checked
NOTE: For some of the reported defects, checkpatch may be able to
mechanically convert to the typical style using --fix or --fix-inplace.
/github/workspace/src/src/13227257.patch has style problems, please review.
NOTE: Ignored message types: UNKNOWN_COMMIT_ID
NOTE: If any of the errors are false positives, please report
them to the maintainer, see CHECKPATCH in MAINTAINERS.
---
Regards,
Linux Bluetooth
On Sun, Apr 30, 2023 at 10:56:59AM -0700, bluez.test.bot@gmail.com wrote: > Test: CheckPatch - FAIL > Desc: Run checkpatch.pl script > Output: > Bluetooth: Fix UAF in hci_conn_hash_flush again > WARNING: Reported-by: should be immediately followed by Link: with a URL to the report > #92: > Reported-by: syzbot+8bb72f86fc823817bc5d@syzkaller.appspotmail.com > Closes: https://syzkaller.appspot.com/bug?extid=8bb72f86fc823817bc5d > > WARNING: Unknown link reference 'Closes:', use 'Link:' instead > #93: > Closes: https://syzkaller.appspot.com/bug?extid=8bb72f86fc823817bc5d > Well, CI is out of date, as the mainline has changed this from 'Link:' to 'Closes:' [1]. [1]: https://git.kernel.org/pub/scm/linux/kernel/git/torvalds/linux.git/commit/?id=44c31888098a590b8ec5ba37009e5a983f7c4b46 > CHECK: Alignment should match open parenthesis > #163: FILE: net/bluetooth/hci_conn.c:2473: > + while ((conn = list_first_entry_or_null(head, > + struct hci_conn, list)) != NULL) { > > total: 0 errors, 2 warnings, 1 checks, 57 lines checked > > NOTE: For some of the reported defects, checkpatch may be able to > mechanically convert to the typical style using --fix or --fix-inplace. > > /github/workspace/src/src/13227257.patch has style problems, please review. > > NOTE: Ignored message types: UNKNOWN_COMMIT_ID > > NOTE: If any of the errors are false positives, please report > them to the maintainer, see CHECKPATCH in MAINTAINERS. Thanks, Ruihan Li
Hi Ruihan, On Sun, Apr 30, 2023 at 11:23 AM Ruihan Li <lrh2000@pku.edu.cn> wrote: > > On Sun, Apr 30, 2023 at 10:56:59AM -0700, bluez.test.bot@gmail.com wrote: > > Test: CheckPatch - FAIL > > Desc: Run checkpatch.pl script > > Output: > > Bluetooth: Fix UAF in hci_conn_hash_flush again > > WARNING: Reported-by: should be immediately followed by Link: with a URL to the report > > #92: > > Reported-by: syzbot+8bb72f86fc823817bc5d@syzkaller.appspotmail.com > > Closes: https://syzkaller.appspot.com/bug?extid=8bb72f86fc823817bc5d > > > > WARNING: Unknown link reference 'Closes:', use 'Link:' instead > > #93: > > Closes: https://syzkaller.appspot.com/bug?extid=8bb72f86fc823817bc5d > > > > Well, CI is out of date, as the mainline has changed this from 'Link:' to > 'Closes:' [1]. > > [1]: https://git.kernel.org/pub/scm/linux/kernel/git/torvalds/linux.git/commit/?id=44c31888098a590b8ec5ba37009e5a983f7c4b46 > > > CHECK: Alignment should match open parenthesis > > #163: FILE: net/bluetooth/hci_conn.c:2473: > > + while ((conn = list_first_entry_or_null(head, > > + struct hci_conn, list)) != NULL) { > > > > total: 0 errors, 2 warnings, 1 checks, 57 lines checked > > > > NOTE: For some of the reported defects, checkpatch may be able to > > mechanically convert to the typical style using --fix or --fix-inplace. > > > > /github/workspace/src/src/13227257.patch has style problems, please review. > > > > NOTE: Ignored message types: UNKNOWN_COMMIT_ID > > > > NOTE: If any of the errors are false positives, please report > > them to the maintainer, see CHECKPATCH in MAINTAINERS. > > Thanks, > Ruihan Li I guess this depends on the order the connection is added into the list since after applying your changes it causes the following trace with iso-tester: ================================================================== BUG: KASAN: slab-use-after-free in hci_conn_unlink (./include/linux/list.h:114 ./include/linux/list.h:137 ./include/linux/rculist.h:157 net/bl Write of size 8 at addr ffff8880012e89d0 by task iso-tester/31 Hardware name: QEMU Standard PC (Q35 + ICH9, 2009), BIOS 1.16.1-2.fc36 04/01/2014 Call Trace: <TASK> dump_stack_lvl (./arch/x86/include/asm/irqflags.h:26 ./arch/x86/include/asm/irqflags.h:67 ./arch/x86/include/asm/irqflags.h:127 lib/dump_stack print_report (mm/kasan/report.c:320 mm/kasan/report.c:430) ? __virt_addr_valid (./include/linux/mmzone.h:1915 ./include/linux/mmzone.h:2011 arch/x86/mm/physaddr.c:65) ? hci_conn_unlink (./include/linux/list.h:114 ./include/linux/list.h:137 ./include/linux/rculist.h:157 net/bluetooth/hci_conn.c:1108) kasan_report (mm/kasan/report.c:538) ? hci_conn_unlink (./include/linux/list.h:114 ./include/linux/list.h:137 ./include/linux/rculist.h:157 net/bluetooth/hci_conn.c:1108) hci_conn_unlink (./include/linux/list.h:114 ./include/linux/list.h:137 ./include/linux/rculist.h:157 net/bluetooth/hci_conn.c:1108) hci_conn_del (./include/linux/instrumented.h:72 ./include/linux/atomic/atomic-instrumented.h:27 ./include/net/bluetooth/hci_core.h:1416 net/bl hci_conn_hash_flush (net/bluetooth/hci_conn.c:2479) hci_dev_close_sync (net/bluetooth/hci_sync.c:4943) hci_unregister_dev (net/bluetooth/hci_core.c:556 net/bluetooth/hci_core.c:2703) vhci_release (drivers/bluetooth/hci_vhci.c:670) __fput (fs/file_table.c:322) task_work_run (kernel/task_work.c:180 (discriminator 1)) ? __pfx_task_work_run (kernel/task_work.c:147) ? mark_held_locks (kernel/locking/lockdep.c:4225) exit_to_user_mode_prepare (./include/linux/resume_user_mode.h:49 kernel/entry/common.c:171 kernel/entry/common.c:204) syscall_exit_to_user_mode (kernel/entry/common.c:130 kernel/entry/common.c:299) do_syscall_64 (arch/x86/entry/common.c:87) entry_SYSCALL_64_after_hwframe (arch/x86/entry/entry_64.S:120) RIP: 0033:0x7f899c2352f7 Code: ff ef 01 00 66 2e 0f 1f 84 00 00 00 00 00 0f 1f 44 00 00 f3 0f 1e fa 64 8b 04 25 18 00 00 00 85 c0 75 10 b8 03 00 00 00 0f 05 <48> 3d 00f All code ======== 0: ff (bad) 1: ef out %eax,(%dx) 2: 01 00 add %eax,(%rax) 4: 66 2e 0f 1f 84 00 00 cs nopw 0x0(%rax,%rax,1) b: 00 00 00 e: 0f 1f 44 00 00 nopl 0x0(%rax,%rax,1) 13: f3 0f 1e fa endbr64 17: 64 8b 04 25 18 00 00 mov %fs:0x18,%eax 1e: 00 1f: 85 c0 test %eax,%eax 21: 75 10 jne 0x33 23: b8 03 00 00 00 mov $0x3,%eax 28: 0f 05 syscall 2a:* 48 rex.W <-- trapping instruction 2b: 3d .byte 0x3d 2c: 0f .byte 0xf Code starting with the faulting instruction =========================================== 0: 48 rex.W 1: 3d .byte 0x3d 2: 0f .byte 0xf RSP: 002b:00007ffc785e3588 EFLAGS: 00000246 ORIG_RAX: 0000000000000003 RAX: 0000000000000000 RBX: 0000000000000000 RCX: 00007f899c2352f7 RDX: 0000000000000000 RSI: 0000000000000000 RDI: 0000000000000007 RBP: 0000000000000000 R08: 0000561e1096d390 R09: 0000000000000030 R10: 0000000000000002 R11: 0000000000000246 R12: 0000000000000001 R13: 00007f899c9b84b0 R14: 0000000000000000 R15: 0000561e10961890 </TASK>
Hi Luiz, On Mon, May 01, 2023 at 02:09:05PM -0700, Luiz Augusto von Dentz wrote: > I guess this depends on the order the connection is added into the > list since after applying your changes it causes the following trace > with iso-tester: > > ================================================================== > BUG: KASAN: slab-use-after-free in hci_conn_unlink > (./include/linux/list.h:114 ./include/linux/list.h:137 > ./include/linux/rculist.h:157 net/bl > Write of size 8 at addr ffff8880012e89d0 by task iso-tester/31 It is so weird that even though KASAN reports a critical BUG, the CI still says that all tests are passed. I think that perhaps this is also something we need to develop a fix. (I missed the KASAN report earlier, sorry.) > Hardware name: QEMU Standard PC (Q35 + ICH9, 2009), BIOS 1.16.1-2.fc36 > 04/01/2014 > Call Trace: > <TASK> > dump_stack_lvl (./arch/x86/include/asm/irqflags.h:26 > ./arch/x86/include/asm/irqflags.h:67 > ./arch/x86/include/asm/irqflags.h:127 lib/dump_stack > print_report (mm/kasan/report.c:320 mm/kasan/report.c:430) > ? __virt_addr_valid (./include/linux/mmzone.h:1915 > ./include/linux/mmzone.h:2011 arch/x86/mm/physaddr.c:65) > ? hci_conn_unlink (./include/linux/list.h:114 > ./include/linux/list.h:137 ./include/linux/rculist.h:157 > net/bluetooth/hci_conn.c:1108) > kasan_report (mm/kasan/report.c:538) > ? hci_conn_unlink (./include/linux/list.h:114 > ./include/linux/list.h:137 ./include/linux/rculist.h:157 > net/bluetooth/hci_conn.c:1108) > hci_conn_unlink (./include/linux/list.h:114 ./include/linux/list.h:137 > ./include/linux/rculist.h:157 net/bluetooth/hci_conn.c:1108) > hci_conn_del (./include/linux/instrumented.h:72 > ./include/linux/atomic/atomic-instrumented.h:27 > ./include/net/bluetooth/hci_core.h:1416 net/bl > hci_conn_hash_flush (net/bluetooth/hci_conn.c:2479) > hci_dev_close_sync (net/bluetooth/hci_sync.c:4943) > hci_unregister_dev (net/bluetooth/hci_core.c:556 net/bluetooth/hci_core.c:2703) > vhci_release (drivers/bluetooth/hci_vhci.c:670) > __fput (fs/file_table.c:322) > task_work_run (kernel/task_work.c:180 (discriminator 1)) > ? __pfx_task_work_run (kernel/task_work.c:147) > ? mark_held_locks (kernel/locking/lockdep.c:4225) > exit_to_user_mode_prepare (./include/linux/resume_user_mode.h:49 > kernel/entry/common.c:171 kernel/entry/common.c:204) > syscall_exit_to_user_mode (kernel/entry/common.c:130 kernel/entry/common.c:299) > do_syscall_64 (arch/x86/entry/common.c:87) > entry_SYSCALL_64_after_hwframe (arch/x86/entry/entry_64.S:120) > RIP: 0033:0x7f899c2352f7 > Code: ff ef 01 00 66 2e 0f 1f 84 00 00 00 00 00 0f 1f 44 00 00 f3 0f > 1e fa 64 8b 04 25 18 00 00 00 85 c0 75 10 b8 03 00 00 00 0f 05 <48> 3d > 00f > All code > ======== > 0: ff (bad) > 1: ef out %eax,(%dx) > 2: 01 00 add %eax,(%rax) > 4: 66 2e 0f 1f 84 00 00 cs nopw 0x0(%rax,%rax,1) > b: 00 00 00 > e: 0f 1f 44 00 00 nopl 0x0(%rax,%rax,1) > 13: f3 0f 1e fa endbr64 > 17: 64 8b 04 25 18 00 00 mov %fs:0x18,%eax > 1e: 00 > 1f: 85 c0 test %eax,%eax > 21: 75 10 jne 0x33 > 23: b8 03 00 00 00 mov $0x3,%eax > 28: 0f 05 syscall > 2a:* 48 rex.W <-- trapping instruction > 2b: 3d .byte 0x3d > 2c: 0f .byte 0xf > > Code starting with the faulting instruction > =========================================== > 0: 48 rex.W > 1: 3d .byte 0x3d > 2: 0f .byte 0xf > RSP: 002b:00007ffc785e3588 EFLAGS: 00000246 ORIG_RAX: 0000000000000003 > RAX: 0000000000000000 RBX: 0000000000000000 RCX: 00007f899c2352f7 > RDX: 0000000000000000 RSI: 0000000000000000 RDI: 0000000000000007 > RBP: 0000000000000000 R08: 0000561e1096d390 R09: 0000000000000030 > R10: 0000000000000002 R11: 0000000000000246 R12: 0000000000000001 > R13: 00007f899c9b84b0 R14: 0000000000000000 R15: 0000561e10961890 > </TASK> This leads to another hci_conn_unlink BUG, as resolved by the following diff: diff --git a/net/bluetooth/hci_conn.c b/net/bluetooth/hci_conn.c index 70e1655a9..44d0643fc 100644 --- a/net/bluetooth/hci_conn.c +++ b/net/bluetooth/hci_conn.c @@ -1102,12 +1102,12 @@ static void hci_conn_unlink(struct hci_conn *conn) if (!conn->link) return; - hci_conn_put(conn->parent); - conn->parent = NULL; - list_del_rcu(&conn->link->list); synchronize_rcu(); + hci_conn_put(conn->parent); + conn->parent = NULL; + kfree(conn->link); conn->link = NULL; } In a word, we cannot perform list_del_rcu(&conn->link->list) after hci_conn_put, because hci_conn_put might have already invalidated the list entry by deallocating its list head (in conn->parent). There are actually quite a few "problems" (I think) with hci_conn_unlink. I tried to fix them and ended up with a patch series of six patches (each patch is fairly small, of course), including two that have already been submitted and the one above. I'll submit these patches later. They have to be a patch series because they all target one function, hci_conn_unlink. So there will be a lot of merge conflicts if they are submitted individually. I'm not intentionally tieing them together, so in case that any patch is inappropriate, please let me know (I'd appreciate if you could explain why). > -- > Luiz Augusto von Dentz Thanks, Ruihan Li
diff --git a/include/net/bluetooth/hci_core.h b/include/net/bluetooth/hci_core.h index a6c8aee2f..8baf34639 100644 --- a/include/net/bluetooth/hci_core.h +++ b/include/net/bluetooth/hci_core.h @@ -1327,7 +1327,7 @@ int hci_le_create_cis(struct hci_conn *conn); struct hci_conn *hci_conn_add(struct hci_dev *hdev, int type, bdaddr_t *dst, u8 role); -int hci_conn_del(struct hci_conn *conn); +void hci_conn_del(struct hci_conn *conn); void hci_conn_hash_flush(struct hci_dev *hdev); void hci_conn_check_pending(struct hci_dev *hdev); diff --git a/net/bluetooth/hci_conn.c b/net/bluetooth/hci_conn.c index 640b951bf..85c34c837 100644 --- a/net/bluetooth/hci_conn.c +++ b/net/bluetooth/hci_conn.c @@ -1109,7 +1109,7 @@ static void hci_conn_unlink(struct hci_conn *conn) hci_conn_del(conn); } -int hci_conn_del(struct hci_conn *conn) +void hci_conn_del(struct hci_conn *conn) { struct hci_dev *hdev = conn->hdev; @@ -1160,8 +1160,6 @@ int hci_conn_del(struct hci_conn *conn) * rest of hci_conn_del. */ hci_conn_cleanup(conn); - - return 0; } struct hci_dev *hci_get_route(bdaddr_t *dst, bdaddr_t *src, uint8_t src_type) @@ -2462,22 +2460,20 @@ void hci_conn_enter_active_mode(struct hci_conn *conn, __u8 force_active) /* Drop all connection on the device */ void hci_conn_hash_flush(struct hci_dev *hdev) { - struct hci_conn_hash *h = &hdev->conn_hash; - struct hci_conn *c, *n; + struct list_head *head = &hdev->conn_hash.list; + struct hci_conn *conn; BT_DBG("hdev %s", hdev->name); - list_for_each_entry_safe(c, n, &h->list, list) { - c->state = BT_CLOSED; - - hci_disconn_cfm(c, HCI_ERROR_LOCAL_HOST_TERM); - - /* Unlink before deleting otherwise it is possible that - * hci_conn_del removes the link which may cause the list to - * contain items already freed. - */ - hci_conn_unlink(c); - hci_conn_del(c); + /* We should not traverse the list here, because hci_conn_del + * can remove extra links, which may cause the link traversal + * to hit items that have already been released. + */ + while ((conn = list_first_entry_or_null(head, + struct hci_conn, list)) != NULL) { + conn->state = BT_CLOSED; + hci_disconn_cfm(conn, HCI_ERROR_LOCAL_HOST_TERM); + hci_conn_del(conn); } }
Commit 06149746e720 ("Bluetooth: hci_conn: Add support for linking multiple hcon") reintroduces a previously fixed bug [1] ("KASAN: slab-use-after-free Read in hci_conn_hash_flush"). This bug was originally fixed by commit 5dc7d23e167e ("Bluetooth: hci_conn: Fix possible UAF"). The hci_conn_unlink function was added to avoid invalidating the link traversal caused by successive hci_conn_del operations releasing extra connections. However, currently hci_conn_unlink itself also releases extra connections, resulted in the reintroduced bug. This patch follows a more robust solution for cleaning up all connections, by repeatedly removing the first connection until there are none left. This approach does not rely on the inner workings of hci_conn_del and ensures proper cleanup of all connections. However, we need to make sure that hci_conn_del never fails. Indeed it doesn't, as it now always returns zero. To make this a bit clearer, this patch also changes its return type to void. Reported-by: syzbot+8bb72f86fc823817bc5d@syzkaller.appspotmail.com Closes: https://syzkaller.appspot.com/bug?extid=8bb72f86fc823817bc5d Fixes: 06149746e720 ("Bluetooth: hci_conn: Add support for linking multiple hcon") Signed-off-by: Ruihan Li <lrh2000@pku.edu.cn> --- include/net/bluetooth/hci_core.h | 2 +- net/bluetooth/hci_conn.c | 28 ++++++++++++---------------- 2 files changed, 13 insertions(+), 17 deletions(-)