[syzbot] [bluetooth?] KASAN: slab-use-after-free Read in hci_send_acl

[syzbot] [bluetooth?] KASAN: slab-use-after-free Read in hci_send_acl

[syzbot]

Hello,

syzbot found the following issue on:

HEAD commit:    7271b2a53042 Add linux-next specific files for 20230818
git tree:       linux-next
console+strace: https://syzkaller.appspot.com/x/log.txt?x=13404b6ba80000
kernel config:  https://syzkaller.appspot.com/x/.config?x=1936af09cdef7dd6
dashboard link: https://syzkaller.appspot.com/bug?extid=a0c80b06ae2cb8895bc4
compiler:       gcc (Debian 12.2.0-14) 12.2.0, GNU ld (GNU Binutils for Debian) 2.40
syz repro:      https://syzkaller.appspot.com/x/repro.syz?x=13506923a80000
C reproducer:   https://syzkaller.appspot.com/x/repro.c?x=147569efa80000

Downloadable assets:
disk image: https://storage.googleapis.com/syzbot-assets/d81109bc02c1/disk-7271b2a5.raw.xz
vmlinux: https://storage.googleapis.com/syzbot-assets/4b3bf8e2a4f7/vmlinux-7271b2a5.xz
kernel image: https://storage.googleapis.com/syzbot-assets/6404cd473c1e/bzImage-7271b2a5.xz

IMPORTANT: if you fix the issue, please add the following tag to the commit:
Reported-by: syzbot+a0c80b06ae2cb8895bc4@syzkaller.appspotmail.com

==================================================================
BUG: KASAN: slab-use-after-free in hci_send_acl+0xdf4/0xf30 net/bluetooth/hci_core.c:3228
Read of size 8 at addr ffff888025635618 by task kworker/1:1/27

CPU: 1 PID: 27 Comm: kworker/1:1 Not tainted 6.5.0-rc6-next-20230818-syzkaller #0
Hardware name: Google Google Compute Engine/Google Compute Engine, BIOS Google 07/26/2023
Workqueue: events l2cap_info_timeout
Call Trace:
 <TASK>
 __dump_stack lib/dump_stack.c:88 [inline]
 dump_stack_lvl+0xd9/0x1b0 lib/dump_stack.c:106
 print_address_description mm/kasan/report.c:364 [inline]
 print_report+0xc4/0x620 mm/kasan/report.c:475
 kasan_report+0xda/0x110 mm/kasan/report.c:588
 hci_send_acl+0xdf4/0xf30 net/bluetooth/hci_core.c:3228
 l2cap_send_cmd+0x6dd/0x920 net/bluetooth/l2cap_core.c:977
 l2cap_send_conn_req+0x1e5/0x260 net/bluetooth/l2cap_core.c:1286
 l2cap_start_connection+0x11e/0x420 net/bluetooth/l2cap_core.c:1514
 l2cap_conn_start+0x7ae/0xa40 net/bluetooth/l2cap_core.c:1661
 process_one_work+0x887/0x15d0 kernel/workqueue.c:2630
 process_scheduled_works kernel/workqueue.c:2703 [inline]
 worker_thread+0x8bb/0x1290 kernel/workqueue.c:2784
 kthread+0x33a/0x430 kernel/kthread.c:388
 ret_from_fork+0x45/0x80 arch/x86/kernel/process.c:147
 ret_from_fork_asm+0x11/0x20 arch/x86/entry/entry_64.S:304
 </TASK>

Allocated by task 5046:
 kasan_save_stack+0x33/0x50 mm/kasan/common.c:45
 kasan_set_track+0x25/0x30 mm/kasan/common.c:52
 ____kasan_kmalloc mm/kasan/common.c:374 [inline]
 __kasan_kmalloc+0xa2/0xb0 mm/kasan/common.c:383
 kmalloc include/linux/slab.h:600 [inline]
 kzalloc include/linux/slab.h:721 [inline]
 hci_chan_create+0xa6/0x3a0 net/bluetooth/hci_conn.c:2691
 l2cap_conn_add.part.0+0x1a/0xdf0 net/bluetooth/l2cap_core.c:7841
 l2cap_conn_add include/net/bluetooth/l2cap.h:866 [inline]
 l2cap_chan_connect+0x15b9/0x2140 net/bluetooth/l2cap_core.c:8053
 bt_6lowpan_connect net/bluetooth/6lowpan.c:894 [inline]
 lowpan_control_write+0x3d6/0x730 net/bluetooth/6lowpan.c:1129
 full_proxy_write+0x124/0x190 fs/debugfs/file.c:236
 vfs_write+0x2a4/0xe40 fs/read_write.c:582
 ksys_write+0x12f/0x250 fs/read_write.c:637
 do_syscall_x64 arch/x86/entry/common.c:50 [inline]
 do_syscall_64+0x38/0xb0 arch/x86/entry/common.c:80
 entry_SYSCALL_64_after_hwframe+0x63/0xcd

Freed by task 50:
 kasan_save_stack+0x33/0x50 mm/kasan/common.c:45
 kasan_set_track+0x25/0x30 mm/kasan/common.c:52
 kasan_save_free_info+0x2b/0x40 mm/kasan/generic.c:522
 ____kasan_slab_free mm/kasan/common.c:236 [inline]
 ____kasan_slab_free+0x15b/0x1b0 mm/kasan/common.c:200
 kasan_slab_free include/linux/kasan.h:164 [inline]
 slab_free_hook mm/slub.c:1800 [inline]
 slab_free_freelist_hook+0x114/0x1e0 mm/slub.c:1826
 slab_free mm/slub.c:3809 [inline]
 __kmem_cache_free+0xb8/0x2f0 mm/slub.c:3822
 hci_chan_list_flush+0x81/0xf0 net/bluetooth/hci_conn.c:2731
 hci_conn_cleanup net/bluetooth/hci_conn.c:152 [inline]
 hci_conn_del+0x1fc/0xd10 net/bluetooth/hci_conn.c:1140
 hci_abort_conn_sync+0xacb/0xe20 net/bluetooth/hci_sync.c:5432
 abort_conn_sync+0x18e/0x3a0 net/bluetooth/hci_conn.c:2878
 hci_cmd_sync_work+0x1a4/0x3c0 net/bluetooth/hci_sync.c:306
 process_one_work+0x887/0x15d0 kernel/workqueue.c:2630
 process_scheduled_works kernel/workqueue.c:2703 [inline]
 worker_thread+0x8bb/0x1290 kernel/workqueue.c:2784
 kthread+0x33a/0x430 kernel/kthread.c:388
 ret_from_fork+0x45/0x80 arch/x86/kernel/process.c:147
 ret_from_fork_asm+0x11/0x20 arch/x86/entry/entry_64.S:304

Last potentially related work creation:
 kasan_save_stack+0x33/0x50 mm/kasan/common.c:45
 __kasan_record_aux_stack+0xbc/0xd0 mm/kasan/generic.c:492
 kvfree_call_rcu+0x70/0xbe0 kernel/rcu/tree.c:3373
 kernfs_unlink_open_file+0x3b6/0x4b0 fs/kernfs/file.c:633
 kernfs_fop_release+0xec/0x1e0 fs/kernfs/file.c:805
 __fput+0x3f7/0xa70 fs/file_table.c:384
 __fput_sync+0x47/0x50 fs/file_table.c:465
 __do_sys_close fs/open.c:1572 [inline]
 __se_sys_close fs/open.c:1557 [inline]
 __x64_sys_close+0x87/0xf0 fs/open.c:1557
 do_syscall_x64 arch/x86/entry/common.c:50 [inline]
 do_syscall_64+0x38/0xb0 arch/x86/entry/common.c:80
 entry_SYSCALL_64_after_hwframe+0x63/0xcd

Second to last potentially related work creation:
 kasan_save_stack+0x33/0x50 mm/kasan/common.c:45
 __kasan_record_aux_stack+0xbc/0xd0 mm/kasan/generic.c:492
 kvfree_call_rcu+0x70/0xbe0 kernel/rcu/tree.c:3373
 kernfs_unlink_open_file+0x3b6/0x4b0 fs/kernfs/file.c:633
 kernfs_fop_release+0xec/0x1e0 fs/kernfs/file.c:805
 __fput+0x3f7/0xa70 fs/file_table.c:384
 __fput_sync+0x47/0x50 fs/file_table.c:465
 __do_sys_close fs/open.c:1572 [inline]
 __se_sys_close fs/open.c:1557 [inline]
 __x64_sys_close+0x87/0xf0 fs/open.c:1557
 do_syscall_x64 arch/x86/entry/common.c:50 [inline]
 do_syscall_64+0x38/0xb0 arch/x86/entry/common.c:80
 entry_SYSCALL_64_after_hwframe+0x63/0xcd

The buggy address belongs to the object at ffff888025635600
 which belongs to the cache kmalloc-128 of size 128
The buggy address is located 24 bytes inside of
 freed 128-byte region [ffff888025635600, ffff888025635680)

The buggy address belongs to the physical page:
page:ffffea0000958d40 refcount:1 mapcount:0 mapping:0000000000000000 index:0x0 pfn:0x25635
flags: 0xfff00000000800(slab|node=0|zone=1|lastcpupid=0x7ff)
page_type: 0xffffffff()
raw: 00fff00000000800 ffff888012c418c0 dead000000000100 dead000000000122
raw: 0000000000000000 0000000000100010 00000001ffffffff 0000000000000000
page dumped because: kasan: bad access detected
page_owner tracks the page as allocated
page last allocated via order 0, migratetype Unmovable, gfp_mask 0x12cc0(GFP_KERNEL|__GFP_NOWARN|__GFP_NORETRY), pid 4500, tgid 4500 (udevd), ts 28013160887, free_ts 27864795775
 set_page_owner include/linux/page_owner.h:31 [inline]
 post_alloc_hook+0x2cf/0x340 mm/page_alloc.c:1536
 prep_new_page mm/page_alloc.c:1543 [inline]
 get_page_from_freelist+0x10d7/0x31b0 mm/page_alloc.c:3219
 __alloc_pages+0x1d0/0x4a0 mm/page_alloc.c:4475
 alloc_pages+0x1a9/0x270 mm/mempolicy.c:2298
 alloc_slab_page mm/slub.c:1870 [inline]
 allocate_slab+0x251/0x380 mm/slub.c:2017
 new_slab mm/slub.c:2070 [inline]
 ___slab_alloc+0x8be/0x1570 mm/slub.c:3223
 __slab_alloc.constprop.0+0x56/0xa0 mm/slub.c:3322
 __slab_alloc_node mm/slub.c:3375 [inline]
 slab_alloc_node mm/slub.c:3468 [inline]
 __kmem_cache_alloc_node+0x137/0x350 mm/slub.c:3517
 kmalloc_trace+0x25/0xe0 mm/slab_common.c:1095
 kmalloc include/linux/slab.h:600 [inline]
 kzalloc include/linux/slab.h:721 [inline]
 kernfs_get_open_node fs/kernfs/file.c:572 [inline]
 kernfs_fop_open+0xad1/0xe00 fs/kernfs/file.c:740
 do_dentry_open+0x88b/0x1730 fs/open.c:929
 do_open fs/namei.c:3636 [inline]
 path_openat+0x19af/0x29c0 fs/namei.c:3793
 do_filp_open+0x1de/0x430 fs/namei.c:3820
 do_sys_openat2+0x176/0x1e0 fs/open.c:1422
 do_sys_open fs/open.c:1437 [inline]
 __do_sys_openat fs/open.c:1453 [inline]
 __se_sys_openat fs/open.c:1448 [inline]
 __x64_sys_openat+0x175/0x210 fs/open.c:1448
 do_syscall_x64 arch/x86/entry/common.c:50 [inline]
 do_syscall_64+0x38/0xb0 arch/x86/entry/common.c:80
page last free stack trace:
 reset_page_owner include/linux/page_owner.h:24 [inline]
 free_pages_prepare mm/page_alloc.c:1136 [inline]
 free_unref_page_prepare+0x476/0xa40 mm/page_alloc.c:2348
 free_unref_page+0x33/0x3b0 mm/page_alloc.c:2441
 qlink_free mm/kasan/quarantine.c:166 [inline]
 qlist_free_all+0x6a/0x170 mm/kasan/quarantine.c:185
 kasan_quarantine_reduce+0x18b/0x1d0 mm/kasan/quarantine.c:292
 __kasan_slab_alloc+0x65/0x90 mm/kasan/common.c:305
 kasan_slab_alloc include/linux/kasan.h:188 [inline]
 slab_post_alloc_hook mm/slab.h:762 [inline]
 slab_alloc_node mm/slub.c:3478 [inline]
 slab_alloc mm/slub.c:3486 [inline]
 __kmem_cache_alloc_lru mm/slub.c:3493 [inline]
 kmem_cache_alloc+0x172/0x3b0 mm/slub.c:3502
 getname_flags.part.0+0x50/0x4d0 fs/namei.c:140
 getname_flags include/linux/audit.h:319 [inline]
 getname+0x90/0xe0 fs/namei.c:219
 do_sys_openat2+0x100/0x1e0 fs/open.c:1416
 do_sys_open fs/open.c:1437 [inline]
 __do_sys_openat fs/open.c:1453 [inline]
 __se_sys_openat fs/open.c:1448 [inline]
 __x64_sys_openat+0x175/0x210 fs/open.c:1448
 do_syscall_x64 arch/x86/entry/common.c:50 [inline]
 do_syscall_64+0x38/0xb0 arch/x86/entry/common.c:80
 entry_SYSCALL_64_after_hwframe+0x63/0xcd

Memory state around the buggy address:
 ffff888025635500: 00 00 00 00 00 00 00 00 00 00 00 00 00 fc fc fc
 ffff888025635580: fc fc fc fc fc fc fc fc fc fc fc fc fc fc fc fc
>ffff888025635600: fa fb fb fb fb fb fb fb fb fb fb fb fb fb fb fb
                            ^
 ffff888025635680: fc fc fc fc fc fc fc fc fc fc fc fc fc fc fc fc
 ffff888025635700: fa fb fb fb fb fb fb fb fb fb fb fb fb fb fb fb
==================================================================


---
This report is generated by a bot. It may contain errors.
See https://goo.gl/tpsmEJ for more information about syzbot.
syzbot engineers can be reached at syzkaller@googlegroups.com.

syzbot will keep track of this issue. See:
https://goo.gl/tpsmEJ#status for how to communicate with syzbot.

If the bug is already fixed, let syzbot know by replying with:
#syz fix: exact-commit-title

If you want syzbot to run the reproducer, reply with:
#syz test: git://repo/address.git branch-or-commit-hash
If you attach or paste a git patch, syzbot will apply it before testing.

If you want to overwrite bug's subsystems, reply with:
#syz set subsystems: new-subsystem
(See the list of subsystem names on the web dashboard)

If the bug is a duplicate of another bug, reply with:
#syz dup: exact-subject-of-another-report

If you want to undo deduplication, reply with:
#syz undup

Re: [syzbot] [bluetooth?] KASAN: slab-use-after-free Read in hci_send_acl

[syzbot]

Hello,

syzbot has tested the proposed patch and the reproducer did not trigger any issue:

Reported-and-tested-by: syzbot+a0c80b06ae2cb8895bc4@syzkaller.appspotmail.com

Tested on:

commit:         2a05334d Bluetooth: btusb: Do not call kfree_skb() und..
git tree:       git://git.kernel.org/pub/scm/linux/kernel/git/bluetooth/bluetooth-next.git master
console output: https://syzkaller.appspot.com/x/log.txt?x=1521e55ba80000
kernel config:  https://syzkaller.appspot.com/x/.config?x=e532e371ba4b65ca
dashboard link: https://syzkaller.appspot.com/bug?extid=a0c80b06ae2cb8895bc4
compiler:       gcc (Debian 12.2.0-14) 12.2.0, GNU ld (GNU Binutils for Debian) 2.40
patch:          https://syzkaller.appspot.com/x/patch.diff?x=11eaff5ba80000

Note: testing is done by a robot and is best-effort only.

Re: [syzbot] [bluetooth?] KASAN: slab-use-after-free Read in hci_send_acl

[syzbot]

syzbot has bisected this issue to:

commit 45c37c4e9c9aab5bb1cf5778d8e5ebd9f9ad820a
Author: Luiz Augusto von Dentz <luiz.von.dentz@intel.com>
Date:   Wed Aug 9 23:49:33 2023 +0000

    Bluetooth: hci_sync: Fix UAF in hci_disconnect_all_sync

bisection log:  https://syzkaller.appspot.com/x/bisect.txt?x=1042c5ffa80000
start commit:   7271b2a53042 Add linux-next specific files for 20230818
git tree:       linux-next
final oops:     https://syzkaller.appspot.com/x/report.txt?x=1242c5ffa80000
console output: https://syzkaller.appspot.com/x/log.txt?x=1442c5ffa80000
kernel config:  https://syzkaller.appspot.com/x/.config?x=1936af09cdef7dd6
dashboard link: https://syzkaller.appspot.com/bug?extid=a0c80b06ae2cb8895bc4
syz repro:      https://syzkaller.appspot.com/x/repro.syz?x=13506923a80000
C reproducer:   https://syzkaller.appspot.com/x/repro.c?x=147569efa80000

Reported-by: syzbot+a0c80b06ae2cb8895bc4@syzkaller.appspotmail.com
Fixes: 45c37c4e9c9a ("Bluetooth: hci_sync: Fix UAF in hci_disconnect_all_sync")

For information about bisection process see: https://goo.gl/tpsmEJ#bisection

[PATCH v2 1/2] Bluetooth: hci_sync: always check if connection is alive before deleting

[Pauli Virtanen]

In hci_abort_conn_sync it is possible that conn is deleted concurrently
by something else, also e.g. when waiting for hdev->lock.  This causes
double deletion of the conn, so UAF or conn_hash.list corruption.

Fix by having all code paths check that the connection is still in
conn_hash before deleting it, while holding hdev->lock which prevents
any races.

Log (when powering off while BAP streaming, occurs rarely):
=======================================================================
kernel BUG at lib/list_debug.c:56!
...
 ? __list_del_entry_valid (lib/list_debug.c:56)
 hci_conn_del (net/bluetooth/hci_conn.c:154) bluetooth
 hci_abort_conn_sync (net/bluetooth/hci_sync.c:5415) bluetooth
 ? __pfx_hci_abort_conn_sync+0x10/0x10 [bluetooth]
 ? lock_release+0x1d5/0x3c0
 ? hci_disconnect_all_sync.constprop.0+0xb2/0x230 [bluetooth]
 ? __pfx_lock_release+0x10/0x10
 ? __kmem_cache_free+0x14d/0x2e0
 hci_disconnect_all_sync.constprop.0+0xda/0x230 [bluetooth]
 ? __pfx_hci_disconnect_all_sync.constprop.0+0x10/0x10 [bluetooth]
 ? hci_clear_adv_sync+0x14f/0x170 [bluetooth]
 ? __pfx_set_powered_sync+0x10/0x10 [bluetooth]
 hci_set_powered_sync+0x293/0x450 [bluetooth]
=======================================================================

Fixes: 94d9ba9f9888 ("Bluetooth: hci_sync: Fix UAF in hci_disconnect_all_sync")
Signed-off-by: Pauli Virtanen <pav@iki.fi>
---

Notes:
    v2: rebased

 net/bluetooth/hci_sync.c | 26 ++++++++++++--------------
 1 file changed, 12 insertions(+), 14 deletions(-)

diff --git a/net/bluetooth/hci_sync.c b/net/bluetooth/hci_sync.c
index 3640d73f9595..c6f57af88bfd 100644
--- a/net/bluetooth/hci_sync.c
+++ b/net/bluetooth/hci_sync.c
@@ -5380,6 +5380,7 @@ int hci_abort_conn_sync(struct hci_dev *hdev, struct hci_conn *conn, u8 reason)
 {
 	int err = 0;
 	u16 handle = conn->handle;
+	bool disconnect = false;
 	struct hci_conn *c;
 
 	switch (conn->state) {
@@ -5395,24 +5396,15 @@ int hci_abort_conn_sync(struct hci_dev *hdev, struct hci_conn *conn, u8 reason)
 		break;
 	case BT_OPEN:
 	case BT_BOUND:
-		hci_dev_lock(hdev);
-		hci_conn_failed(conn, reason);
-		hci_dev_unlock(hdev);
-		return 0;
+		break;
 	default:
-		hci_dev_lock(hdev);
-		conn->state = BT_CLOSED;
-		hci_disconn_cfm(conn, reason);
-		hci_conn_del(conn);
-		hci_dev_unlock(hdev);
-		return 0;
+		disconnect = true;
+		break;
 	}
 
 	hci_dev_lock(hdev);
 
-	/* Check if the connection hasn't been cleanup while waiting
-	 * commands to complete.
-	 */
+	/* Check if the connection has been cleaned up concurrently */
 	c = hci_conn_hash_lookup_handle(hdev, handle);
 	if (!c || c != conn) {
 		err = 0;
@@ -5424,7 +5416,13 @@ int hci_abort_conn_sync(struct hci_dev *hdev, struct hci_conn *conn, u8 reason)
 	 * or in case of LE it was still scanning so it can be cleanup
 	 * safely.
 	 */
-	hci_conn_failed(conn, reason);
+	if (disconnect) {
+		conn->state = BT_CLOSED;
+		hci_disconn_cfm(conn, reason);
+		hci_conn_del(conn);
+	} else {
+		hci_conn_failed(conn, reason);
+	}
 
 unlock:
 	hci_dev_unlock(hdev);
-- 
2.41.0

[PATCH v2 2/2] Bluetooth: hci_conn: verify connection is to be aborted before doing it

[Pauli Virtanen]

There is a race condition where a connection handle is reused, after
hci_abort_conn but before abort_conn_sync is processed in hci_sync. In
this case, hci_abort_conn_sync ends up working on the wrong connection,
which can have abort_reason==0, in which case hci_connect_cfm is called
with success status and then connection is deleted, which causes various
use-after-free.

Fix by checking abort_reason is nonzero before calling
hci_abort_conn_sync. If it's zero, then the connection is probably a
different one than we expected and should not be aborted.

Also fix some theoretical UAF / races, where something frees the conn
while hci_abort_conn_sync is working on it.

Fixes: a13f316e90fd ("Bluetooth: hci_conn: Consolidate code for aborting connections")
Reported-by: syzbot+a0c80b06ae2cb8895bc4@syzkaller.appspotmail.com
Closes: https://lore.kernel.org/linux-bluetooth/0000000000005ab984060371583e@google.com/
Signed-off-by: Pauli Virtanen <pav@iki.fi>
---

Notes:
    v2: drop one probably not necessary WARN_ON

    Not sure how you'd hit this condition in real controller, but syzbot
    does end up calling hci_abort_conn_sync with reason == 0 which then
    causes havoc.

    This can be verified: with a patch that changes abort_conn_sync to

        2874	conn = hci_conn_hash_lookup_handle(hdev, handle);
        2875	if (!conn || WARN_ON(!conn->abort_reason))
        2876		return 0;

    https://syzkaller.appspot.com/text?tag=Patch&x=16eff740680000

    it hits that WARN_ON:

    https://syzkaller.appspot.com/x/log.txt?x=10affb97a80000

#syz test git://git.kernel.org/pub/scm/linux/kernel/git/bluetooth/bluetooth-next.git master

 net/bluetooth/hci_conn.c | 17 ++++++++++++++++-
 1 file changed, 16 insertions(+), 1 deletion(-)

diff --git a/net/bluetooth/hci_conn.c b/net/bluetooth/hci_conn.c
index e62a5f368a51..22de82071e35 100644
--- a/net/bluetooth/hci_conn.c
+++ b/net/bluetooth/hci_conn.c
@@ -2901,12 +2901,25 @@ static int abort_conn_sync(struct hci_dev *hdev, void *data)
 {
 	struct hci_conn *conn;
 	u16 handle = PTR_UINT(data);
+	u8 reason;
+	int err;
+
+	rcu_read_lock();
 
 	conn = hci_conn_hash_lookup_handle(hdev, handle);
+	if (conn) {
+		reason = READ_ONCE(conn->abort_reason);
+		conn = reason ? hci_conn_get(conn) : NULL;
+	}
+
+	rcu_read_unlock();
+
 	if (!conn)
 		return 0;
 
-	return hci_abort_conn_sync(hdev, conn, conn->abort_reason);
+	err = hci_abort_conn_sync(hdev, conn, reason);
+	hci_conn_put(conn);
+	return err;
 }
 
 int hci_abort_conn(struct hci_conn *conn, u8 reason)
@@ -2918,6 +2931,8 @@ int hci_abort_conn(struct hci_conn *conn, u8 reason)
 	 */
 	if (conn->abort_reason)
 		return 0;
+	if (WARN_ON(!reason))
+		reason = HCI_ERROR_UNSPECIFIED;
 
 	bt_dev_dbg(hdev, "handle 0x%2.2x reason 0x%2.2x", conn->handle, reason);
 
-- 
2.41.0

Re: [syzbot] [bluetooth?] KASAN: slab-use-after-free Read in hci_send_acl

[syzbot]

Hello,

syzbot has tested the proposed patch and the reproducer did not trigger any issue:

Reported-and-tested-by: syzbot+a0c80b06ae2cb8895bc4@syzkaller.appspotmail.com

Tested on:

commit:         62dc2425 Bluetooth: ISO: Fix invalid context error
git tree:       git://git.kernel.org/pub/scm/linux/kernel/git/bluetooth/bluetooth-next.git master
console output: https://syzkaller.appspot.com/x/log.txt?x=12897062680000
kernel config:  https://syzkaller.appspot.com/x/.config?x=3d83e69abefedb6e
dashboard link: https://syzkaller.appspot.com/bug?extid=a0c80b06ae2cb8895bc4
compiler:       Debian clang version 15.0.6, GNU ld (GNU Binutils for Debian) 2.40
userspace arch: arm64
patch:          https://syzkaller.appspot.com/x/patch.diff?x=14a7711a680000

Note: testing is done by a robot and is best-effort only.

RE: [v2,1/2] Bluetooth: hci_sync: always check if connection is alive before deleting

[bluez.test.bot]

[-- Attachment #1: Type: text/plain, Size: 2942 bytes --]

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=789014

---Test result---

Test Summary:
CheckPatch                    PASS      1.53 seconds
GitLint                       FAIL      0.86 seconds
SubjectPrefix                 PASS      0.20 seconds
BuildKernel                   PASS      33.69 seconds
CheckAllWarning               PASS      36.16 seconds
CheckSparse                   PASS      41.46 seconds
CheckSmatch                   PASS      115.45 seconds
BuildKernel32                 PASS      31.93 seconds
TestRunnerSetup               PASS      489.12 seconds
TestRunner_l2cap-tester       PASS      30.32 seconds
TestRunner_iso-tester         PASS      49.69 seconds
TestRunner_bnep-tester        PASS      9.79 seconds
TestRunner_mgmt-tester        PASS      214.41 seconds
TestRunner_rfcomm-tester      PASS      14.96 seconds
TestRunner_sco-tester         PASS      18.53 seconds
TestRunner_ioctl-tester       PASS      16.93 seconds
TestRunner_mesh-tester        PASS      12.59 seconds
TestRunner_smp-tester         PASS      13.38 seconds
TestRunner_userchan-tester    PASS      10.31 seconds
IncrementalBuild              PASS      37.23 seconds

Details
##############################
Test: GitLint - FAIL
Desc: Run gitlint
Output:
[v2,1/2] Bluetooth: hci_sync: always check if connection is alive before deleting

WARNING: I3 - ignore-body-lines: gitlint will be switching from using Python regex 'match' (match beginning) to 'search' (match anywhere) semantics. Please review your ignore-body-lines.regex option accordingly. To remove this warning, set general.regex-style-search=True. More details: https://jorisroovers.github.io/gitlint/configuration/#regex-style-search
1: T1 Title exceeds max length (81>80): "[v2,1/2] Bluetooth: hci_sync: always check if connection is alive before deleting"
[v2,2/2] Bluetooth: hci_conn: verify connection is to be aborted before doing it

WARNING: I3 - ignore-body-lines: gitlint will be switching from using Python regex 'match' (match beginning) to 'search' (match anywhere) semantics. Please review your ignore-body-lines.regex option accordingly. To remove this warning, set general.regex-style-search=True. More details: https://jorisroovers.github.io/gitlint/configuration/#regex-style-search
18: B1 Line exceeds max length (88>80): "Closes: https://lore.kernel.org/linux-bluetooth/0000000000005ab984060371583e@google.com/"
31: B3 Line contains hard tab characters (\t): "        2874	conn = hci_conn_hash_lookup_handle(hdev, handle);"
32: B3 Line contains hard tab characters (\t): "        2875	if (!conn || WARN_ON(!conn->abort_reason))"
33: B3 Line contains hard tab characters (\t): "        2876		return 0;"


---
Regards,
Linux Bluetooth

Re: [PATCH v2 2/2] Bluetooth: hci_conn: verify connection is to be aborted before doing it

[Luiz Augusto von Dentz]

Hi Pauli,

On Sat, Sep 30, 2023 at 3:25 PM Pauli Virtanen <pav@iki.fi> wrote:
>
> There is a race condition where a connection handle is reused, after
> hci_abort_conn but before abort_conn_sync is processed in hci_sync. In
> this case, hci_abort_conn_sync ends up working on the wrong connection,
> which can have abort_reason==0, in which case hci_connect_cfm is called
> with success status and then connection is deleted, which causes various
> use-after-free.
>
> Fix by checking abort_reason is nonzero before calling
> hci_abort_conn_sync. If it's zero, then the connection is probably a
> different one than we expected and should not be aborted.
>
> Also fix some theoretical UAF / races, where something frees the conn
> while hci_abort_conn_sync is working on it.
>
> Fixes: a13f316e90fd ("Bluetooth: hci_conn: Consolidate code for aborting connections")
> Reported-by: syzbot+a0c80b06ae2cb8895bc4@syzkaller.appspotmail.com
> Closes: https://lore.kernel.org/linux-bluetooth/0000000000005ab984060371583e@google.com/
> Signed-off-by: Pauli Virtanen <pav@iki.fi>
> ---
>
> Notes:
>     v2: drop one probably not necessary WARN_ON
>
>     Not sure how you'd hit this condition in real controller, but syzbot
>     does end up calling hci_abort_conn_sync with reason == 0 which then
>     causes havoc.
>
>     This can be verified: with a patch that changes abort_conn_sync to
>
>         2874    conn = hci_conn_hash_lookup_handle(hdev, handle);
>         2875    if (!conn || WARN_ON(!conn->abort_reason))
>         2876            return 0;
>
>     https://syzkaller.appspot.com/text?tag=Patch&x=16eff740680000
>
>     it hits that WARN_ON:
>
>     https://syzkaller.appspot.com/x/log.txt?x=10affb97a80000
>
> #syz test git://git.kernel.org/pub/scm/linux/kernel/git/bluetooth/bluetooth-next.git master
>
>  net/bluetooth/hci_conn.c | 17 ++++++++++++++++-
>  1 file changed, 16 insertions(+), 1 deletion(-)
>
> diff --git a/net/bluetooth/hci_conn.c b/net/bluetooth/hci_conn.c
> index e62a5f368a51..22de82071e35 100644
> --- a/net/bluetooth/hci_conn.c
> +++ b/net/bluetooth/hci_conn.c
> @@ -2901,12 +2901,25 @@ static int abort_conn_sync(struct hci_dev *hdev, void *data)
>  {
>         struct hci_conn *conn;
>         u16 handle = PTR_UINT(data);
> +       u8 reason;
> +       int err;
> +
> +       rcu_read_lock();
>
>         conn = hci_conn_hash_lookup_handle(hdev, handle);
> +       if (conn) {
> +               reason = READ_ONCE(conn->abort_reason);
> +               conn = reason ? hci_conn_get(conn) : NULL;
> +       }
> +
> +       rcu_read_unlock();
> +
>         if (!conn)
>                 return 0;
>
> -       return hci_abort_conn_sync(hdev, conn, conn->abort_reason);
> +       err = hci_abort_conn_sync(hdev, conn, reason);
> +       hci_conn_put(conn);
> +       return err;
>  }
>
>  int hci_abort_conn(struct hci_conn *conn, u8 reason)
> @@ -2918,6 +2931,8 @@ int hci_abort_conn(struct hci_conn *conn, u8 reason)
>          */
>         if (conn->abort_reason)
>                 return 0;
> +       if (WARN_ON(!reason))
> +               reason = HCI_ERROR_UNSPECIFIED;
>
>         bt_dev_dbg(hdev, "handle 0x%2.2x reason 0x%2.2x", conn->handle, reason);
>
> --
> 2.41.0

Don't really like where this is going, the culprit here seems that we
are not able to cancel the callback when freeing the hci_conn so it
stays enqueued while the new connection is created with the same
handle, so I think we need to introduce a function that cancel queued
like Ive sent sometime back, that way we don't need to keep trying to
check if it is the same connection or not.


-- 
Luiz Augusto von Dentz

Re: [PATCH v2 2/2] Bluetooth: hci_conn: verify connection is to be aborted before doing it

[Pauli Virtanen]

Hi Luiz,

ma, 2023-10-02 kello 16:22 -0700, Luiz Augusto von Dentz kirjoitti:
> Hi Pauli,
> 
> On Sat, Sep 30, 2023 at 3:25 PM Pauli Virtanen <pav@iki.fi> wrote:
> > 
> > There is a race condition where a connection handle is reused, after
> > hci_abort_conn but before abort_conn_sync is processed in hci_sync. In
> > this case, hci_abort_conn_sync ends up working on the wrong connection,
> > which can have abort_reason==0, in which case hci_connect_cfm is called
> > with success status and then connection is deleted, which causes various
> > use-after-free.
> > 
> > Fix by checking abort_reason is nonzero before calling
> > hci_abort_conn_sync. If it's zero, then the connection is probably a
> > different one than we expected and should not be aborted.
> > 
> > Also fix some theoretical UAF / races, where something frees the conn
> > while hci_abort_conn_sync is working on it.
> > 
> > Fixes: a13f316e90fd ("Bluetooth: hci_conn: Consolidate code for aborting connections")
> > Reported-by: syzbot+a0c80b06ae2cb8895bc4@syzkaller.appspotmail.com
> > Closes: https://lore.kernel.org/linux-bluetooth/0000000000005ab984060371583e@google.com/
> > Signed-off-by: Pauli Virtanen <pav@iki.fi>
> > ---
> > 
> > Notes:
> >     v2: drop one probably not necessary WARN_ON
> > 
> >     Not sure how you'd hit this condition in real controller, but syzbot
> >     does end up calling hci_abort_conn_sync with reason == 0 which then
> >     causes havoc.
> > 
> >     This can be verified: with a patch that changes abort_conn_sync to
> > 
> >         2874    conn = hci_conn_hash_lookup_handle(hdev, handle);
> >         2875    if (!conn || WARN_ON(!conn->abort_reason))
> >         2876            return 0;
> > 
> >     https://syzkaller.appspot.com/text?tag=Patch&x=16eff740680000
> > 
> >     it hits that WARN_ON:
> > 
> >     https://syzkaller.appspot.com/x/log.txt?x=10affb97a80000
> > 
> > #syz test git://git.kernel.org/pub/scm/linux/kernel/git/bluetooth/bluetooth-next.git master
> > 
> >  net/bluetooth/hci_conn.c | 17 ++++++++++++++++-
> >  1 file changed, 16 insertions(+), 1 deletion(-)
> > 
> > diff --git a/net/bluetooth/hci_conn.c b/net/bluetooth/hci_conn.c
> > index e62a5f368a51..22de82071e35 100644
> > --- a/net/bluetooth/hci_conn.c
> > +++ b/net/bluetooth/hci_conn.c
> > @@ -2901,12 +2901,25 @@ static int abort_conn_sync(struct hci_dev *hdev, void *data)
> >  {
> >         struct hci_conn *conn;
> >         u16 handle = PTR_UINT(data);
> > +       u8 reason;
> > +       int err;
> > +
> > +       rcu_read_lock();
> > 
> >         conn = hci_conn_hash_lookup_handle(hdev, handle);
> > +       if (conn) {
> > +               reason = READ_ONCE(conn->abort_reason);
> > +               conn = reason ? hci_conn_get(conn) : NULL;
> > +       }
> > +
> > +       rcu_read_unlock();
> > +
> >         if (!conn)
> >                 return 0;
> > 
> > -       return hci_abort_conn_sync(hdev, conn, conn->abort_reason);
> > +       err = hci_abort_conn_sync(hdev, conn, reason);
> > +       hci_conn_put(conn);
> > +       return err;
> >  }
> > 
> >  int hci_abort_conn(struct hci_conn *conn, u8 reason)
> > @@ -2918,6 +2931,8 @@ int hci_abort_conn(struct hci_conn *conn, u8 reason)
> >          */
> >         if (conn->abort_reason)
> >                 return 0;
> > +       if (WARN_ON(!reason))
> > +               reason = HCI_ERROR_UNSPECIFIED;
> > 
> >         bt_dev_dbg(hdev, "handle 0x%2.2x reason 0x%2.2x", conn->handle, reason);
> > 
> > --
> > 2.41.0
> 
> Don't really like where this is going, the culprit here seems that we
> are not able to cancel the callback when freeing the hci_conn so it
> stays enqueued while the new connection is created with the same
> handle, so I think we need to introduce a function that cancel queued
> like Ive sent sometime back, that way we don't need to keep trying to
> check if it is the same connection or not.

Ack.

Note though that the first patch in series is separate issue, and won't
be fixed by canceling callbacks.

Removing the item from hci_sync queue on deletion would also avoid
syzbot hitting the issue in this second patch.

There'd be a remaining theoretical race though in

	conn = hci_conn_hash_lookup_handle(hdev, handle);
	/* the other workqueue frees conn here -> UAF */
	if (!conn)
		return 0;

unless you do the other stuff here. I wonder if there instead should be
something like hci_cmd_sync_dev_lock/unlock from
https://lore.kernel.org/linux-bluetooth/15e7863c06bd87cd991ab963132fa9d12ef7e11a.1690399379.git.pav@iki.fi/
to limit the concurrency only to the places where we wait for
controller events.

-- 
Pauli Virtanen

Re: [PATCH v2 1/2] Bluetooth: hci_sync: always check if connection is alive before deleting

[patchwork-bot+bluetooth]

Hello:

This series was applied to bluetooth/bluetooth-next.git (master)
by Luiz Augusto von Dentz <luiz.von.dentz@intel.com>:

On Sat, 30 Sep 2023 15:53:32 +0300 you wrote:
> In hci_abort_conn_sync it is possible that conn is deleted concurrently
> by something else, also e.g. when waiting for hdev->lock.  This causes
> double deletion of the conn, so UAF or conn_hash.list corruption.
> 
> Fix by having all code paths check that the connection is still in
> conn_hash before deleting it, while holding hdev->lock which prevents
> any races.
> 
> [...]

Here is the summary with links:
  - [v2,1/2] Bluetooth: hci_sync: always check if connection is alive before deleting
    https://git.kernel.org/bluetooth/bluetooth-next/c/32f6776f0083
  - [v2,2/2] Bluetooth: hci_conn: verify connection is to be aborted before doing it
    (no matching commit)

You are awesome, thank you!
-- 
Deet-doot-dot, I am a bot.
https://korg.docs.kernel.org/patchwork/pwbot.html

Re: [syzbot] [bluetooth?] KASAN: slab-use-after-free Read in hci_send_acl

[syzbot]

syzbot suspects this issue was fixed by commit:

commit 181a42edddf51d5d9697ecdf365d72ebeab5afb0
Author: Ziyang Xuan <william.xuanziyang@huawei.com>
Date:   Wed Oct 11 09:57:31 2023 +0000

    Bluetooth: Make handle of hci_conn be unique

bisection log:  https://syzkaller.appspot.com/x/bisect.txt?x=127944c1e80000
start commit:   4b2b606075e5 ipv4/fib: send notify when delete source addr..
git tree:       net
kernel config:  https://syzkaller.appspot.com/x/.config?x=d594086f139d167
dashboard link: https://syzkaller.appspot.com/bug?extid=a0c80b06ae2cb8895bc4
syz repro:      https://syzkaller.appspot.com/x/repro.syz?x=138aad9e680000
C reproducer:   https://syzkaller.appspot.com/x/repro.c?x=125e0b92680000

If the result looks correct, please mark the issue as fixed by replying with:

#syz fix: Bluetooth: Make handle of hci_conn be unique

For information about bisection process see: https://goo.gl/tpsmEJ#bisection