* [syzbot] BUG: corrupted list in kobject_add_internal (4)
@ 2022-09-15 1:17 syzbot
2022-09-17 1:47 ` Hawkins Jiawei
0 siblings, 1 reply; 8+ messages in thread
From: syzbot @ 2022-09-15 1:17 UTC (permalink / raw)
To: gregkh, linux-kernel, rafael, syzkaller-bugs
Hello,
syzbot found the following issue on:
HEAD commit: b96fbd602d35 Merge tag 's390-6.0-4' of git://git.kernel.or..
git tree: upstream
console+strace: https://syzkaller.appspot.com/x/log.txt?x=1226a500880000
kernel config: https://syzkaller.appspot.com/x/.config?x=c79df237bd9a0448
dashboard link: https://syzkaller.appspot.com/bug?extid=e653e3f67251b6139aaa
compiler: gcc (Debian 10.2.1-6) 10.2.1 20210110, GNU ld (GNU Binutils for Debian) 2.35.2
syz repro: https://syzkaller.appspot.com/x/repro.syz?x=10f618e8880000
C reproducer: https://syzkaller.appspot.com/x/repro.c?x=1090704f080000
IMPORTANT: if you fix the issue, please add the following tag to the commit:
Reported-by: syzbot+e653e3f67251b6139aaa@syzkaller.appspotmail.com
list_add double add: new=ffff88807cd08540, prev=ffff88807cd08540, next=ffff888144a30000.
------------[ cut here ]------------
kernel BUG at lib/list_debug.c:33!
invalid opcode: 0000 [#1] PREEMPT SMP KASAN
CPU: 0 PID: 3610 Comm: kworker/u5:2 Not tainted 6.0.0-rc4-syzkaller-00302-gb96fbd602d35 #0
Hardware name: Google Google Compute Engine/Google Compute Engine, BIOS Google 08/26/2022
Workqueue: hci0 hci_rx_work
RIP: 0010:__list_add_valid.cold+0x42/0x58 lib/list_debug.c:33
Code: e8 73 f6 f0 ff 0f 0b 48 c7 c7 40 f9 48 8a e8 65 f6 f0 ff 0f 0b 48 89 f2 4c 89 e1 48 89 ee 48 c7 c7 40 fb 48 8a e8 4e f6 f0 ff <0f> 0b 48 89 f1 48 c7 c7 c0 fa 48 8a 4c 89 e6 e8 3a f6 f0 ff 0f 0b
RSP: 0018:ffffc9000398f800 EFLAGS: 00010282
RAX: 0000000000000058 RBX: ffff8880216bd298 RCX: 0000000000000000
RDX: ffff888076949d80 RSI: ffffffff8161f408 RDI: fffff52000731ef2
RBP: ffff88807cd08540 R08: 0000000000000058 R09: 0000000000000000
R10: 0000000080000001 R11: 0000000000000000 R12: ffff888144a30000
R13: ffff88807cd08550 R14: ffff88807cd08558 R15: ffff88807cd08540
FS: 0000000000000000(0000) GS:ffff8880b9a00000(0000) knlGS:0000000000000000
CS: 0010 DS: 0000 ES: 0000 CR0: 0000000080050033
CR2: 0000000020000000 CR3: 0000000078c49000 CR4: 00000000003506f0
DR0: 0000000000000000 DR1: 0000000000000000 DR2: 0000000000000000
DR3: 0000000000000000 DR6: 00000000fffe0ff0 DR7: 0000000000000400
Call Trace:
<TASK>
__list_add include/linux/list.h:69 [inline]
list_add_tail include/linux/list.h:102 [inline]
kobj_kset_join lib/kobject.c:164 [inline]
kobject_add_internal+0x18f/0x8f0 lib/kobject.c:214
kobject_add_varg lib/kobject.c:358 [inline]
kobject_add+0x150/0x1c0 lib/kobject.c:410
device_add+0x368/0x1e90 drivers/base/core.c:3452
hci_conn_add_sysfs+0x9b/0x1b0 net/bluetooth/hci_sysfs.c:53
hci_le_cis_estabilished_evt+0x57c/0xae0 net/bluetooth/hci_event.c:6799
hci_le_meta_evt+0x2b8/0x510 net/bluetooth/hci_event.c:7110
hci_event_func net/bluetooth/hci_event.c:7440 [inline]
hci_event_packet+0x63d/0xfd0 net/bluetooth/hci_event.c:7495
hci_rx_work+0xae7/0x1230 net/bluetooth/hci_core.c:4007
process_one_work+0x991/0x1610 kernel/workqueue.c:2289
worker_thread+0x665/0x1080 kernel/workqueue.c:2436
kthread+0x2e4/0x3a0 kernel/kthread.c:376
ret_from_fork+0x1f/0x30 arch/x86/entry/entry_64.S:306
</TASK>
Modules linked in:
---[ end trace 0000000000000000 ]---
RIP: 0010:__list_add_valid.cold+0x42/0x58 lib/list_debug.c:33
Code: e8 73 f6 f0 ff 0f 0b 48 c7 c7 40 f9 48 8a e8 65 f6 f0 ff 0f 0b 48 89 f2 4c 89 e1 48 89 ee 48 c7 c7 40 fb 48 8a e8 4e f6 f0 ff <0f> 0b 48 89 f1 48 c7 c7 c0 fa 48 8a 4c 89 e6 e8 3a f6 f0 ff 0f 0b
RSP: 0018:ffffc9000398f800 EFLAGS: 00010282
RAX: 0000000000000058 RBX: ffff8880216bd298 RCX: 0000000000000000
RDX: ffff888076949d80 RSI: ffffffff8161f408 RDI: fffff52000731ef2
RBP: ffff88807cd08540 R08: 0000000000000058 R09: 0000000000000000
R10: 0000000080000001 R11: 0000000000000000 R12: ffff888144a30000
R13: ffff88807cd08550 R14: ffff88807cd08558 R15: ffff88807cd08540
FS: 0000000000000000(0000) GS:ffff8880b9a00000(0000) knlGS:0000000000000000
CS: 0010 DS: 0000 ES: 0000 CR0: 0000000080050033
CR2: 0000000020000000 CR3: 000000000bc8e000 CR4: 00000000003506f0
DR0: 0000000000000000 DR1: 0000000000000000 DR2: 0000000000000000
DR3: 0000000000000000 DR6: 00000000fffe0ff0 DR7: 0000000000000400
---
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.
syzbot can test patches for this issue, for details see:
https://goo.gl/tpsmEJ#testing-patches
^ permalink raw reply [flat|nested] 8+ messages in thread
* Re: [syzbot] BUG: corrupted list in kobject_add_internal (4)
2022-09-15 1:17 [syzbot] BUG: corrupted list in kobject_add_internal (4) syzbot
@ 2022-09-17 1:47 ` Hawkins Jiawei
2022-09-19 8:41 ` Hawkins Jiawei
0 siblings, 1 reply; 8+ messages in thread
From: Hawkins Jiawei @ 2022-09-17 1:47 UTC (permalink / raw)
To: syzbot+e653e3f67251b6139aaa
Cc: gregkh, linux-kernel, rafael, syzkaller-bugs, soenke.huster,
linux-bluetooth, luiz.dentz, marcel, 18801353760, yin31149
Hi,
On Fri, 26 Aug 2022 08:27:06, AM Sönke Huster <soenke.huster@eknoes.de> wrote:
>Hi Luiz,
>
>On 25.08.22 20:58, Luiz Augusto von Dentz wrote:
>> Hi Sönke,
>>
>> On Thu, Aug 25, 2022 at 8:08 AM Sönke Huster <soenke.huster@eknoes.de> wrote:
>>>
>>> Hi,
>>>
>>>
>>>
>>> While fuzzing I found several crashes similar to the following:
>>>
>>>
>>> [ 5.345731] sysfs: cannot create duplicate filename '/devices/virtual/bluetooth/hci0/hci0:0'
>>>
>>> [...]
>>>
>>> [ 5.430464] BUG: KASAN: use-after-free in klist_add_tail+0x1bd/0x200
>>>
>>> [ 5.430464] Write of size 8 at addr ffff88800bfcc468 by task kworker/u3:1/43
>>>
>>> [ 5.430464]
>>>
>>> [ 5.430464] CPU: 0 PID: 43 Comm: kworker/u3:1 Not tainted 5.19.0-12855-g13f222680b8f #2
>>>
>>> [ 5.430464] Hardware name: QEMU Standard PC (i440FX + PIIX, 1996), BIOS rel-1.16.0-0-gd239552ce722-prebuilt.qemu.org 04/01/2014
>>>
>>> [ 5.430464] Workqueue: hci0 hci_rx_work
>>>
>>> [ 5.430464] Call Trace:
>>>
>>> [ 5.430464] <TASK>
>>>
>>> [ 5.430464] dump_stack_lvl+0x45/0x5d
>>>
>>> [ 5.430464] print_report.cold+0x5e/0x5e5
>>>
>>> [ 5.430464] kasan_report+0xb1/0x1c0
>>>
>>> [ 5.430464] klist_add_tail+0x1bd/0x200
>>>
>>> [ 5.430464] device_add+0xa6b/0x1b80
>>>
>>> [ 5.430464] hci_conn_add_sysfs+0x91/0x110
>>>
>>> [ 5.430464] le_conn_complete_evt+0x117f/0x17d0
>>>
>>> [ 5.430464] hci_le_conn_complete_evt+0x226/0x2c0
>>>
>>> [ 5.430464] hci_le_meta_evt+0x2c0/0x4a0
>>>
>>> [ 5.430464] hci_event_packet+0x636/0xf60
>>>
>>> [ 5.430464] hci_rx_work+0xa8c/0x1000
>>>
>>> [ 5.430464] process_one_work+0x8df/0x1530
>>>
>>> [ 5.430464] worker_thread+0x575/0x11a0
>>>
>>> [ 5.430464] kthread+0x29d/0x340
>>>
>>> [ 5.430464] ret_from_fork+0x22/0x30
>>>
>>> [ 5.430464] </TASK>
>>>
>>> [ 5.430464]
>>>
>>> [ 5.430464] Allocated by task 44:
>>>
>>> [ 5.430464] kasan_save_stack+0x1e/0x40
>>>
>>> [ 5.430464] __kasan_kmalloc+0x81/0xa0
>>>
>>> [ 5.430464] device_add+0xcae/0x1b80
>>>
>>> [ 5.430464] hci_conn_add_sysfs+0x91/0x110
>>>
>>> [ 5.430464] le_conn_complete_evt+0x117f/0x17d0
>>>
>>> [ 5.430464] hci_le_conn_complete_evt+0x226/0x2c0
>>>
>>> [ 5.430464] hci_le_meta_evt+0x2c0/0x4a0
>>>
>>> [ 5.430464] hci_event_packet+0x636/0xf60
>>>
>>> [ 5.430464] hci_rx_work+0xa8c/0x1000
>>>
>>> [ 5.430464] process_one_work+0x8df/0x1530
>>>
>>> [ 5.430464] worker_thread+0x575/0x11a0
>>>
>>> [ 5.430464] kthread+0x29d/0x340
>>>
>>> [ 5.430464] ret_from_fork+0x22/0x30
>>>
>>> [ 5.430464]
>>>
>>> [ 5.430464] Freed by task 43:
>>>
>>> [ 5.430464] kasan_save_stack+0x1e/0x40
>>>
>>> [ 5.430464] kasan_set_track+0x21/0x30
>>>
>>> [ 5.430464] kasan_set_free_info+0x20/0x40
>>>
>>> [ 5.430464] __kasan_slab_free+0x108/0x190
>>>
>>> [ 5.430464] kfree+0xa9/0x360
>>>
>>> [ 5.430464] device_add+0x33a/0x1b80
>>>
>>> [ 5.430464] hci_conn_add_sysfs+0x91/0x110
>>>
>>> [ 5.430464] hci_le_cis_estabilished_evt+0x517/0xa70
>>>
>>> [ 5.430464] hci_le_meta_evt+0x2c0/0x4a0
>>>
>>> [ 5.430464] hci_event_packet+0x636/0xf60
>>>
>>> [ 5.430464] hci_rx_work+0xa8c/0x1000
>>>
>>> [ 5.430464] process_one_work+0x8df/0x1530
>>>
>>> [ 5.430464] worker_thread+0x575/0x11a0
>>>
>>> [ 5.430464] kthread+0x29d/0x340
>>>
>>> [ 5.430464] ret_from_fork+0x22/0x30
>>>
>>>
>>>
>>> I think I fixed a similar issue in d5ebaa7c5f6f ("Bluetooth: hci_event: Ignore multiple conn complete events"). Here, the problem was that multiple connection complete events for the same handle called hci_conn_add_sysfs multiple times, but if it is called with an existing connection conn->dev->p is freed.
>>>
>>> This is because device_add is called - its documentation contains this sentence: "Do not call this routine or device_register() more than once for any device structure".
>>>
>>>
>>>
>>> This here is similar: First, an hci_le_conn_complete_evt creates a new connection.
>>>
>>> Afterwards, an hci_le_cis_estabilished_evt with the same handle finds that connection, and tries to add it to sysfs again, freeing conn->dev->p. Now, an event that might use that connection such as here the hci_le_conn_complete_evt triggers a use after free.
>>>
Syzkaller reports a bug as follows [1]:
------------[ cut here ]------------
kernel BUG at lib/list_debug.c:33!
invalid opcode: 0000 [#1] PREEMPT SMP KASAN
[...]
Call Trace:
<TASK>
__list_add include/linux/list.h:69 [inline]
list_add_tail include/linux/list.h:102 [inline]
kobj_kset_join lib/kobject.c:164 [inline]
kobject_add_internal+0x18f/0x8f0 lib/kobject.c:214
kobject_add_varg lib/kobject.c:358 [inline]
kobject_add+0x150/0x1c0 lib/kobject.c:410
device_add+0x368/0x1e90 drivers/base/core.c:3452
hci_conn_add_sysfs+0x9b/0x1b0 net/bluetooth/hci_sysfs.c:53
hci_le_cis_estabilished_evt+0x57c/0xae0 net/bluetooth/hci_event.c:6799
hci_le_meta_evt+0x2b8/0x510 net/bluetooth/hci_event.c:7110
hci_event_func net/bluetooth/hci_event.c:7440 [inline]
hci_event_packet+0x63d/0xfd0 net/bluetooth/hci_event.c:7495
hci_rx_work+0xae7/0x1230 net/bluetooth/hci_core.c:4007
process_one_work+0x991/0x1610 kernel/workqueue.c:2289
worker_thread+0x665/0x1080 kernel/workqueue.c:2436
kthread+0x2e4/0x3a0 kernel/kthread.c:376
ret_from_fork+0x1f/0x30 arch/x86/entry/entry_64.S:306
</TASK>
I think they are the same problems:
A hci_le_conn_complete_evt creates a new connection, and calls
hci_conn_add_sysfs(). Then hci_le_cis_estabilished_evt with the same handle
finds that connection, and will also calls hci_conn_add_sysfs(), which maybe
triggering corrupted list bug.
Link: https://syzkaller.appspot.com/bug?id=da3246e2d33afdb92d66bc166a0934c5b146404a [1]
>>>
>>>
>>> I bisected this bug and it was introduced with 26afbd826ee3 ("Bluetooth: Add initial implementation of CIS connections").
>>>
>>>
>>>
>>> The same happens with hci_le_create_big_complete_evt: Here, multiple events trigger the following bug:
>>>
>>>
>>>
>>> [ 6.898080] BUG: kernel NULL pointer dereference, address: 0000000000000058
>>>
>>> [ 6.898081] #PF: supervisor read access in kernel mode
>>>
>>> [ 6.898083] #PF: error_code(0x0000) - not-present page
>>>
>>> [ 6.898085] Oops: 0000 [#1] PREEMPT SMP NOPTI
>>>
>>> [ 6.898090] Workqueue: hci0 hci_rx_work
>>>
>>> [ 6.898092] RIP: 0010:klist_next+0x12/0x160
>>>
>>> [ 6.898128] Call Trace:
>>>
>>> [ 6.898129] <TASK>
>>>
>>> [ 6.898130] ? bt_link_release+0x20/0x20
>>>
>>> [ 6.898133] device_find_child+0x37/0xa0
>>>
>>> [ 6.898136] hci_conn_del_sysfs+0x71/0xa0
>>>
>>> [ 6.898138] hci_conn_cleanup+0x17a/0x2c0
>>>
>>> [ 6.898141] hci_conn_del+0x14a/0x240
>>>
>>> [ 6.898144] hci_le_create_big_complete_evt+0x3d8/0x470
>>>
>>> [ 6.898146] ? hci_le_remote_feat_complete_evt+0x3e0/0x3e0
>>>
>>> [ 6.898148] hci_le_meta_evt+0x155/0x230
>>>
>>> [ 6.898150] hci_event_packet+0x328/0x820
>>>
>>> [ 6.898152] ? hci_conn_drop+0x100/0x100
>>>
>>> [ 6.898155] hci_rx_work+0x725/0xb70
>>>
>>> [ 6.898157] process_one_work+0x2a6/0x5d0
>>>
>>> [ 6.898160] worker_thread+0x4a/0x3e0
>>>
>>> [ 6.898162] ? process_one_work+0x5d0/0x5d0
>>>
>>> [ 6.898164] kthread+0xed/0x120
>>>
>>> [ 6.898165] ? kthread_complete_and_exit+0x20/0x20
>>>
>>> [ 6.898167] ret_from_fork+0x22/0x30
>>>
>>> [ 6.898170] </TASK>
>>>
>>>
>>>
>>> I bisected this bug and it was introduced with eca0ae4aea66 ("Bluetooth: Add initial implementation of BIS connections").
>>>
>>>
>>>
>>> I am not really sure how to solve that. As far as I understand, previously we simply set an unused handle as connection handle, and checked for that before setting the correct handle and adding it to sysfs. But here, adding it to sysfs seems to happen in a different function and the handle is already set.
>>
>> We should probably check if link-type, if it is an ISO link it shall
>> not be created via Connection Complete events and they have their own
>> events to create that.
>>
>
>But then the problem of duplicate hci_le_cis_estabilished_evt / hci_le_create_big_complete_evt still exists, isn't it? For example if the connection is created through a hci_le_cis_req_evt, and afterwards two hci_le_cis_estabilished_evt arrive, the second event calls hci_conn_add_sysfs a second time which frees parts of the device structure.
>
>>> Best
>>> Sönke
I wonder that if we need both two patches? Because they
seems to be used to patch different bugs?
^ permalink raw reply [flat|nested] 8+ messages in thread
* Re: [syzbot] BUG: corrupted list in kobject_add_internal (4)
2022-09-17 1:47 ` Hawkins Jiawei
@ 2022-09-19 8:41 ` Hawkins Jiawei
2022-09-19 16:55 ` Luiz Augusto von Dentz
0 siblings, 1 reply; 8+ messages in thread
From: Hawkins Jiawei @ 2022-09-19 8:41 UTC (permalink / raw)
To: yin31149, syzbot+5a2d2b4a8ca80ad216a9
Cc: 18801353760, gregkh, linux-bluetooth, linux-kernel, luiz.dentz,
marcel, rafael, soenke.huster, syzbot+e653e3f67251b6139aaa,
syzkaller-bugs
On Sat, 17 Sept 2022 at 09:47, Hawkins Jiawei <yin31149@gmail.com> wrote:
>
>Hi,
>
>On Fri, 26 Aug 2022 08:27:06, AM Sönke Huster <soenke.huster@eknoes.de> wrote:
>>Hi Luiz,
>>
>>On 25.08.22 20:58, Luiz Augusto von Dentz wrote:
>>> Hi Sönke,
>>>
>>> On Thu, Aug 25, 2022 at 8:08 AM Sönke Huster <soenke.huster@eknoes.de> wrote:
>>>>
>>>> Hi,
>>>>
>>>>
>>>>
>>>> While fuzzing I found several crashes similar to the following:
>>>>
>>>>
>>>> [ 5.345731] sysfs: cannot create duplicate filename '/devices/virtual/bluetooth/hci0/hci0:0'
>>>>
>>>> [...]
>>>>
>>>> [ 5.430464] BUG: KASAN: use-after-free in klist_add_tail+0x1bd/0x200
>>>>
>>>> [ 5.430464] Write of size 8 at addr ffff88800bfcc468 by task kworker/u3:1/43
>>>>
>>>> [ 5.430464]
>>>>
>>>> [ 5.430464] CPU: 0 PID: 43 Comm: kworker/u3:1 Not tainted 5.19.0-12855-g13f222680b8f #2
>>>>
>>>> [ 5.430464] Hardware name: QEMU Standard PC (i440FX + PIIX, 1996), BIOS rel-1.16.0-0-gd239552ce722-prebuilt.qemu.org 04/01/2014
>>>>
>>>> [ 5.430464] Workqueue: hci0 hci_rx_work
>>>>
>>>> [ 5.430464] Call Trace:
>>>>
>>>> [ 5.430464] <TASK>
>>>>
>>>> [ 5.430464] dump_stack_lvl+0x45/0x5d
>>>>
>>>> [ 5.430464] print_report.cold+0x5e/0x5e5
>>>>
>>>> [ 5.430464] kasan_report+0xb1/0x1c0
>>>>
>>>> [ 5.430464] klist_add_tail+0x1bd/0x200
>>>>
>>>> [ 5.430464] device_add+0xa6b/0x1b80
>>>>
>>>> [ 5.430464] hci_conn_add_sysfs+0x91/0x110
>>>>
>>>> [ 5.430464] le_conn_complete_evt+0x117f/0x17d0
>>>>
>>>> [ 5.430464] hci_le_conn_complete_evt+0x226/0x2c0
>>>>
>>>> [ 5.430464] hci_le_meta_evt+0x2c0/0x4a0
>>>>
>>>> [ 5.430464] hci_event_packet+0x636/0xf60
>>>>
>>>> [ 5.430464] hci_rx_work+0xa8c/0x1000
>>>>
>>>> [ 5.430464] process_one_work+0x8df/0x1530
>>>>
>>>> [ 5.430464] worker_thread+0x575/0x11a0
>>>>
>>>> [ 5.430464] kthread+0x29d/0x340
>>>>
>>>> [ 5.430464] ret_from_fork+0x22/0x30
>>>>
>>>> [ 5.430464] </TASK>
>>>>
>>>> [ 5.430464]
>>>>
>>>> [ 5.430464] Allocated by task 44:
>>>>
>>>> [ 5.430464] kasan_save_stack+0x1e/0x40
>>>>
>>>> [ 5.430464] __kasan_kmalloc+0x81/0xa0
>>>>
>>>> [ 5.430464] device_add+0xcae/0x1b80
>>>>
>>>> [ 5.430464] hci_conn_add_sysfs+0x91/0x110
>>>>
>>>> [ 5.430464] le_conn_complete_evt+0x117f/0x17d0
>>>>
>>>> [ 5.430464] hci_le_conn_complete_evt+0x226/0x2c0
>>>>
>>>> [ 5.430464] hci_le_meta_evt+0x2c0/0x4a0
>>>>
>>>> [ 5.430464] hci_event_packet+0x636/0xf60
>>>>
>>>> [ 5.430464] hci_rx_work+0xa8c/0x1000
>>>>
>>>> [ 5.430464] process_one_work+0x8df/0x1530
>>>>
>>>> [ 5.430464] worker_thread+0x575/0x11a0
>>>>
>>>> [ 5.430464] kthread+0x29d/0x340
>>>>
>>>> [ 5.430464] ret_from_fork+0x22/0x30
>>>>
>>>> [ 5.430464]
>>>>
>>>> [ 5.430464] Freed by task 43:
>>>>
>>>> [ 5.430464] kasan_save_stack+0x1e/0x40
>>>>
>>>> [ 5.430464] kasan_set_track+0x21/0x30
>>>>
>>>> [ 5.430464] kasan_set_free_info+0x20/0x40
>>>>
>>>> [ 5.430464] __kasan_slab_free+0x108/0x190
>>>>
>>>> [ 5.430464] kfree+0xa9/0x360
>>>>
>>>> [ 5.430464] device_add+0x33a/0x1b80
>>>>
>>>> [ 5.430464] hci_conn_add_sysfs+0x91/0x110
>>>>
>>>> [ 5.430464] hci_le_cis_estabilished_evt+0x517/0xa70
>>>>
>>>> [ 5.430464] hci_le_meta_evt+0x2c0/0x4a0
>>>>
>>>> [ 5.430464] hci_event_packet+0x636/0xf60
>>>>
>>>> [ 5.430464] hci_rx_work+0xa8c/0x1000
>>>>
>>>> [ 5.430464] process_one_work+0x8df/0x1530
>>>>
>>>> [ 5.430464] worker_thread+0x575/0x11a0
>>>>
>>>> [ 5.430464] kthread+0x29d/0x340
>>>>
>>>> [ 5.430464] ret_from_fork+0x22/0x30
>>>>
>>>>
>>>>
>>>> I think I fixed a similar issue in d5ebaa7c5f6f ("Bluetooth: hci_event: Ignore multiple conn complete events"). Here, the problem was that multiple connection complete events for the same handle called hci_conn_add_sysfs multiple times, but if it is called with an existing connection conn->dev->p is freed.
>>>>
>>>> This is because device_add is called - its documentation contains this sentence: "Do not call this routine or device_register() more than once for any device structure".
>>>>
>>>>
>>>>
>>>> This here is similar: First, an hci_le_conn_complete_evt creates a new connection.
>>>>
>>>> Afterwards, an hci_le_cis_estabilished_evt with the same handle finds that connection, and tries to add it to sysfs again, freeing conn->dev->p. Now, an event that might use that connection such as here the hci_le_conn_complete_evt triggers a use after free.
>>>>
>
>Syzkaller reports a bug as follows [1]:
>------------[ cut here ]------------
>kernel BUG at lib/list_debug.c:33!
>invalid opcode: 0000 [#1] PREEMPT SMP KASAN
>[...]
>Call Trace:
> <TASK>
> __list_add include/linux/list.h:69 [inline]
> list_add_tail include/linux/list.h:102 [inline]
> kobj_kset_join lib/kobject.c:164 [inline]
> kobject_add_internal+0x18f/0x8f0 lib/kobject.c:214
> kobject_add_varg lib/kobject.c:358 [inline]
> kobject_add+0x150/0x1c0 lib/kobject.c:410
> device_add+0x368/0x1e90 drivers/base/core.c:3452
> hci_conn_add_sysfs+0x9b/0x1b0 net/bluetooth/hci_sysfs.c:53
> hci_le_cis_estabilished_evt+0x57c/0xae0 net/bluetooth/hci_event.c:6799
> hci_le_meta_evt+0x2b8/0x510 net/bluetooth/hci_event.c:7110
> hci_event_func net/bluetooth/hci_event.c:7440 [inline]
> hci_event_packet+0x63d/0xfd0 net/bluetooth/hci_event.c:7495
> hci_rx_work+0xae7/0x1230 net/bluetooth/hci_core.c:4007
> process_one_work+0x991/0x1610 kernel/workqueue.c:2289
> worker_thread+0x665/0x1080 kernel/workqueue.c:2436
> kthread+0x2e4/0x3a0 kernel/kthread.c:376
> ret_from_fork+0x1f/0x30 arch/x86/entry/entry_64.S:306
> </TASK>
>
>I think they are the same problems:
>A hci_le_conn_complete_evt creates a new connection, and calls
>hci_conn_add_sysfs(). Then hci_le_cis_estabilished_evt with the same handle
>finds that connection, and will also calls hci_conn_add_sysfs(), which maybe
>triggering corrupted list bug.
>
>Link: https://syzkaller.appspot.com/bug?id=da3246e2d33afdb92d66bc166a0934c5b146404a [1]
>
>>>>
>>>>
>>>> I bisected this bug and it was introduced with 26afbd826ee3 ("Bluetooth: Add initial implementation of CIS connections").
>>>>
>>>>
>>>>
>>>> The same happens with hci_le_create_big_complete_evt: Here, multiple events trigger the following bug:
>>>>
>>>>
>>>>
>>>> [ 6.898080] BUG: kernel NULL pointer dereference, address: 0000000000000058
>>>>
>>>> [ 6.898081] #PF: supervisor read access in kernel mode
>>>>
>>>> [ 6.898083] #PF: error_code(0x0000) - not-present page
>>>>
>>>> [ 6.898085] Oops: 0000 [#1] PREEMPT SMP NOPTI
>>>>
>>>> [ 6.898090] Workqueue: hci0 hci_rx_work
>>>>
>>>> [ 6.898092] RIP: 0010:klist_next+0x12/0x160
>>>>
>>>> [ 6.898128] Call Trace:
>>>>
>>>> [ 6.898129] <TASK>
>>>>
>>>> [ 6.898130] ? bt_link_release+0x20/0x20
>>>>
>>>> [ 6.898133] device_find_child+0x37/0xa0
>>>>
>>>> [ 6.898136] hci_conn_del_sysfs+0x71/0xa0
>>>>
>>>> [ 6.898138] hci_conn_cleanup+0x17a/0x2c0
>>>>
>>>> [ 6.898141] hci_conn_del+0x14a/0x240
>>>>
>>>> [ 6.898144] hci_le_create_big_complete_evt+0x3d8/0x470
>>>>
>>>> [ 6.898146] ? hci_le_remote_feat_complete_evt+0x3e0/0x3e0
>>>>
>>>> [ 6.898148] hci_le_meta_evt+0x155/0x230
>>>>
>>>> [ 6.898150] hci_event_packet+0x328/0x820
>>>>
>>>> [ 6.898152] ? hci_conn_drop+0x100/0x100
>>>>
>>>> [ 6.898155] hci_rx_work+0x725/0xb70
>>>>
>>>> [ 6.898157] process_one_work+0x2a6/0x5d0
>>>>
>>>> [ 6.898160] worker_thread+0x4a/0x3e0
>>>>
>>>> [ 6.898162] ? process_one_work+0x5d0/0x5d0
>>>>
>>>> [ 6.898164] kthread+0xed/0x120
>>>>
>>>> [ 6.898165] ? kthread_complete_and_exit+0x20/0x20
>>>>
>>>> [ 6.898167] ret_from_fork+0x22/0x30
>>>>
>>>> [ 6.898170] </TASK>
>>>>
>>>>
>>>>
>>>> I bisected this bug and it was introduced with eca0ae4aea66 ("Bluetooth: Add initial implementation of BIS connections").
>>>>
>>>>
>>>>
>>>> I am not really sure how to solve that. As far as I understand, previously we simply set an unused handle as connection handle, and checked for that before setting the correct handle and adding it to sysfs. But here, adding it to sysfs seems to happen in a different function and the handle is already set.
>>>
>>> We should probably check if link-type, if it is an ISO link it shall
>>> not be created via Connection Complete events and they have their own
>>> events to create that.
I wonder if we can check the connection type in hci_le_create_big_complete_evt()
and hci_le_cis_estabilished_evt(), as below:
diff --git a/net/bluetooth/hci_event.c b/net/bluetooth/hci_event.c
index 6643c9c20fa4..5b83473d51b5 100644
--- a/net/bluetooth/hci_event.c
+++ b/net/bluetooth/hci_event.c
@@ -6795,8 +6795,16 @@ static void hci_le_cis_estabilished_evt(struct hci_dev *hdev, void *data,
if (!ev->status) {
conn->state = BT_CONNECTED;
- hci_debugfs_create_conn(conn);
- hci_conn_add_sysfs(conn);
+
+ /* Only ISO_LINK link type need to register connection device
+ * here, others will register in their relative
+ * Connection Complete events
+ */
+ if (conn->type == ISO_LINK) {
+ hci_debugfs_create_conn(conn);
+ hci_conn_add_sysfs(conn);
+ }
+
hci_iso_setup_path(conn);
goto unlock;
}
@@ -6901,8 +6909,16 @@ static void hci_le_create_big_complete_evt(struct hci_dev *hdev, void *data,
if (!ev->status) {
conn->state = BT_CONNECTED;
- hci_debugfs_create_conn(conn);
- hci_conn_add_sysfs(conn);
+
+ /* Only ISO_LINK link type need to register connection device
+ * here, others will register in their relative
+ * Connection Complete events
+ */
+ if (conn->type == ISO_LINK) {
+ hci_debugfs_create_conn(conn);
+ hci_conn_add_sysfs(conn);
+ }
+
hci_iso_setup_path(conn);
goto unlock;
}
It seems that this patch can pass the syzbot test.
Link: https://lore.kernel.org/all/0000000000004f9ca105e8ba8157@google.com/
Reported-and-tested-by: syzbot+5a2d2b4a8ca80ad216a9@syzkaller.appspotmail.com
Link: https://lore.kernel.org/all/0000000000008a7a3f05e8ad02f6@google.com/
Reported-and-tested-by: syzbot+e653e3f67251b6139aaa@syzkaller.appspotmail.com
>>>
>>
>>But then the problem of duplicate hci_le_cis_estabilished_evt / hci_le_create_big_complete_evt still exists, isn't it? For example if the connection is created through a hci_le_cis_req_evt, and afterwards two hci_le_cis_estabilished_evt arrive, the second event calls hci_conn_add_sysfs a second time which frees parts of the device structure.
As for this problem, I wonder if we can check the connection state in those
functions as below, liking patch
d5ebaa7c5f6f("Bluetooth: hci_event: Ignore multiple conn complete events"):
diff --git a/net/bluetooth/hci_event.c b/net/bluetooth/hci_event.c
index 5b83473d51b5..f6b62cfcf082 100644
--- a/net/bluetooth/hci_event.c
+++ b/net/bluetooth/hci_event.c
@@ -6794,6 +6794,14 @@ static void hci_le_cis_estabilished_evt(struct hci_dev *hdev, void *data,
}
if (!ev->status) {
+ /* The HCI_LE_CIS_Estabilished event is only sent once per connection.
+ * Processing it more than once per connection can corrupt kernel memory.
+ *
+ * As the connection state is set here for the first time, it indicates
+ * whether the connection is already set up.
+ */
+ if (conn->state == BT_CONNECTED)
+ goto unlock;
conn->state = BT_CONNECTED;
/* Only ISO_LINK link type need to register connection device
@@ -6908,6 +6916,14 @@ static void hci_le_create_big_complete_evt(struct hci_dev *hdev, void *data,
conn->handle = __le16_to_cpu(ev->bis_handle[0]);
if (!ev->status) {
+ /* The HCI_LE_Create_BIG_Complete event is only sent once per connection.
+ * Processing it more than once per connection can corrupt kernel memory.
+ *
+ * As the connection state is set here for the first time, it indicates
+ * whether the connection is already set up.
+ */
+ if (conn->state == BT_CONNECTED)
+ goto unlock;
conn->state = BT_CONNECTED;
/* Only ISO_LINK link type need to register connection device
>>
>>>> Best
>>>> Sönke
>I wonder that if we need both two patches? Because they
>seems to be used to patch different bugs?
^ permalink raw reply related [flat|nested] 8+ messages in thread
* Re: [syzbot] BUG: corrupted list in kobject_add_internal (4)
2022-09-19 8:41 ` Hawkins Jiawei
@ 2022-09-19 16:55 ` Luiz Augusto von Dentz
2022-09-20 5:10 ` Hawkins Jiawei
0 siblings, 1 reply; 8+ messages in thread
From: Luiz Augusto von Dentz @ 2022-09-19 16:55 UTC (permalink / raw)
To: Hawkins Jiawei
Cc: syzbot+5a2d2b4a8ca80ad216a9, 18801353760, gregkh,
linux-bluetooth, linux-kernel, marcel, rafael, soenke.huster,
syzbot+e653e3f67251b6139aaa, syzkaller-bugs
Hi Hawkins,
On Mon, Sep 19, 2022 at 1:42 AM Hawkins Jiawei <yin31149@gmail.com> wrote:
>
> On Sat, 17 Sept 2022 at 09:47, Hawkins Jiawei <yin31149@gmail.com> wrote:
> >
> >Hi,
> >
> >On Fri, 26 Aug 2022 08:27:06, AM Sönke Huster <soenke.huster@eknoes.de> wrote:
> >>Hi Luiz,
> >>
> >>On 25.08.22 20:58, Luiz Augusto von Dentz wrote:
> >>> Hi Sönke,
> >>>
> >>> On Thu, Aug 25, 2022 at 8:08 AM Sönke Huster <soenke.huster@eknoes.de> wrote:
> >>>>
> >>>> Hi,
> >>>>
> >>>>
> >>>>
> >>>> While fuzzing I found several crashes similar to the following:
> >>>>
> >>>>
> >>>> [ 5.345731] sysfs: cannot create duplicate filename '/devices/virtual/bluetooth/hci0/hci0:0'
> >>>>
> >>>> [...]
> >>>>
> >>>> [ 5.430464] BUG: KASAN: use-after-free in klist_add_tail+0x1bd/0x200
> >>>>
> >>>> [ 5.430464] Write of size 8 at addr ffff88800bfcc468 by task kworker/u3:1/43
> >>>>
> >>>> [ 5.430464]
> >>>>
> >>>> [ 5.430464] CPU: 0 PID: 43 Comm: kworker/u3:1 Not tainted 5.19.0-12855-g13f222680b8f #2
> >>>>
> >>>> [ 5.430464] Hardware name: QEMU Standard PC (i440FX + PIIX, 1996), BIOS rel-1.16.0-0-gd239552ce722-prebuilt.qemu.org 04/01/2014
> >>>>
> >>>> [ 5.430464] Workqueue: hci0 hci_rx_work
> >>>>
> >>>> [ 5.430464] Call Trace:
> >>>>
> >>>> [ 5.430464] <TASK>
> >>>>
> >>>> [ 5.430464] dump_stack_lvl+0x45/0x5d
> >>>>
> >>>> [ 5.430464] print_report.cold+0x5e/0x5e5
> >>>>
> >>>> [ 5.430464] kasan_report+0xb1/0x1c0
> >>>>
> >>>> [ 5.430464] klist_add_tail+0x1bd/0x200
> >>>>
> >>>> [ 5.430464] device_add+0xa6b/0x1b80
> >>>>
> >>>> [ 5.430464] hci_conn_add_sysfs+0x91/0x110
> >>>>
> >>>> [ 5.430464] le_conn_complete_evt+0x117f/0x17d0
> >>>>
> >>>> [ 5.430464] hci_le_conn_complete_evt+0x226/0x2c0
> >>>>
> >>>> [ 5.430464] hci_le_meta_evt+0x2c0/0x4a0
> >>>>
> >>>> [ 5.430464] hci_event_packet+0x636/0xf60
> >>>>
> >>>> [ 5.430464] hci_rx_work+0xa8c/0x1000
> >>>>
> >>>> [ 5.430464] process_one_work+0x8df/0x1530
> >>>>
> >>>> [ 5.430464] worker_thread+0x575/0x11a0
> >>>>
> >>>> [ 5.430464] kthread+0x29d/0x340
> >>>>
> >>>> [ 5.430464] ret_from_fork+0x22/0x30
> >>>>
> >>>> [ 5.430464] </TASK>
> >>>>
> >>>> [ 5.430464]
> >>>>
> >>>> [ 5.430464] Allocated by task 44:
> >>>>
> >>>> [ 5.430464] kasan_save_stack+0x1e/0x40
> >>>>
> >>>> [ 5.430464] __kasan_kmalloc+0x81/0xa0
> >>>>
> >>>> [ 5.430464] device_add+0xcae/0x1b80
> >>>>
> >>>> [ 5.430464] hci_conn_add_sysfs+0x91/0x110
> >>>>
> >>>> [ 5.430464] le_conn_complete_evt+0x117f/0x17d0
> >>>>
> >>>> [ 5.430464] hci_le_conn_complete_evt+0x226/0x2c0
> >>>>
> >>>> [ 5.430464] hci_le_meta_evt+0x2c0/0x4a0
> >>>>
> >>>> [ 5.430464] hci_event_packet+0x636/0xf60
> >>>>
> >>>> [ 5.430464] hci_rx_work+0xa8c/0x1000
> >>>>
> >>>> [ 5.430464] process_one_work+0x8df/0x1530
> >>>>
> >>>> [ 5.430464] worker_thread+0x575/0x11a0
> >>>>
> >>>> [ 5.430464] kthread+0x29d/0x340
> >>>>
> >>>> [ 5.430464] ret_from_fork+0x22/0x30
> >>>>
> >>>> [ 5.430464]
> >>>>
> >>>> [ 5.430464] Freed by task 43:
> >>>>
> >>>> [ 5.430464] kasan_save_stack+0x1e/0x40
> >>>>
> >>>> [ 5.430464] kasan_set_track+0x21/0x30
> >>>>
> >>>> [ 5.430464] kasan_set_free_info+0x20/0x40
> >>>>
> >>>> [ 5.430464] __kasan_slab_free+0x108/0x190
> >>>>
> >>>> [ 5.430464] kfree+0xa9/0x360
> >>>>
> >>>> [ 5.430464] device_add+0x33a/0x1b80
> >>>>
> >>>> [ 5.430464] hci_conn_add_sysfs+0x91/0x110
> >>>>
> >>>> [ 5.430464] hci_le_cis_estabilished_evt+0x517/0xa70
> >>>>
> >>>> [ 5.430464] hci_le_meta_evt+0x2c0/0x4a0
> >>>>
> >>>> [ 5.430464] hci_event_packet+0x636/0xf60
> >>>>
> >>>> [ 5.430464] hci_rx_work+0xa8c/0x1000
> >>>>
> >>>> [ 5.430464] process_one_work+0x8df/0x1530
> >>>>
> >>>> [ 5.430464] worker_thread+0x575/0x11a0
> >>>>
> >>>> [ 5.430464] kthread+0x29d/0x340
> >>>>
> >>>> [ 5.430464] ret_from_fork+0x22/0x30
> >>>>
> >>>>
> >>>>
> >>>> I think I fixed a similar issue in d5ebaa7c5f6f ("Bluetooth: hci_event: Ignore multiple conn complete events"). Here, the problem was that multiple connection complete events for the same handle called hci_conn_add_sysfs multiple times, but if it is called with an existing connection conn->dev->p is freed.
> >>>>
> >>>> This is because device_add is called - its documentation contains this sentence: "Do not call this routine or device_register() more than once for any device structure".
> >>>>
> >>>>
> >>>>
> >>>> This here is similar: First, an hci_le_conn_complete_evt creates a new connection.
> >>>>
> >>>> Afterwards, an hci_le_cis_estabilished_evt with the same handle finds that connection, and tries to add it to sysfs again, freeing conn->dev->p. Now, an event that might use that connection such as here the hci_le_conn_complete_evt triggers a use after free.
> >>>>
> >
> >Syzkaller reports a bug as follows [1]:
> >------------[ cut here ]------------
> >kernel BUG at lib/list_debug.c:33!
> >invalid opcode: 0000 [#1] PREEMPT SMP KASAN
> >[...]
> >Call Trace:
> > <TASK>
> > __list_add include/linux/list.h:69 [inline]
> > list_add_tail include/linux/list.h:102 [inline]
> > kobj_kset_join lib/kobject.c:164 [inline]
> > kobject_add_internal+0x18f/0x8f0 lib/kobject.c:214
> > kobject_add_varg lib/kobject.c:358 [inline]
> > kobject_add+0x150/0x1c0 lib/kobject.c:410
> > device_add+0x368/0x1e90 drivers/base/core.c:3452
> > hci_conn_add_sysfs+0x9b/0x1b0 net/bluetooth/hci_sysfs.c:53
> > hci_le_cis_estabilished_evt+0x57c/0xae0 net/bluetooth/hci_event.c:6799
> > hci_le_meta_evt+0x2b8/0x510 net/bluetooth/hci_event.c:7110
> > hci_event_func net/bluetooth/hci_event.c:7440 [inline]
> > hci_event_packet+0x63d/0xfd0 net/bluetooth/hci_event.c:7495
> > hci_rx_work+0xae7/0x1230 net/bluetooth/hci_core.c:4007
> > process_one_work+0x991/0x1610 kernel/workqueue.c:2289
> > worker_thread+0x665/0x1080 kernel/workqueue.c:2436
> > kthread+0x2e4/0x3a0 kernel/kthread.c:376
> > ret_from_fork+0x1f/0x30 arch/x86/entry/entry_64.S:306
> > </TASK>
> >
> >I think they are the same problems:
> >A hci_le_conn_complete_evt creates a new connection, and calls
> >hci_conn_add_sysfs(). Then hci_le_cis_estabilished_evt with the same handle
> >finds that connection, and will also calls hci_conn_add_sysfs(), which maybe
> >triggering corrupted list bug.
> >
> >Link: https://syzkaller.appspot.com/bug?id=da3246e2d33afdb92d66bc166a0934c5b146404a [1]
> >
> >>>>
> >>>>
> >>>> I bisected this bug and it was introduced with 26afbd826ee3 ("Bluetooth: Add initial implementation of CIS connections").
> >>>>
> >>>>
> >>>>
> >>>> The same happens with hci_le_create_big_complete_evt: Here, multiple events trigger the following bug:
> >>>>
> >>>>
> >>>>
> >>>> [ 6.898080] BUG: kernel NULL pointer dereference, address: 0000000000000058
> >>>>
> >>>> [ 6.898081] #PF: supervisor read access in kernel mode
> >>>>
> >>>> [ 6.898083] #PF: error_code(0x0000) - not-present page
> >>>>
> >>>> [ 6.898085] Oops: 0000 [#1] PREEMPT SMP NOPTI
> >>>>
> >>>> [ 6.898090] Workqueue: hci0 hci_rx_work
> >>>>
> >>>> [ 6.898092] RIP: 0010:klist_next+0x12/0x160
> >>>>
> >>>> [ 6.898128] Call Trace:
> >>>>
> >>>> [ 6.898129] <TASK>
> >>>>
> >>>> [ 6.898130] ? bt_link_release+0x20/0x20
> >>>>
> >>>> [ 6.898133] device_find_child+0x37/0xa0
> >>>>
> >>>> [ 6.898136] hci_conn_del_sysfs+0x71/0xa0
> >>>>
> >>>> [ 6.898138] hci_conn_cleanup+0x17a/0x2c0
> >>>>
> >>>> [ 6.898141] hci_conn_del+0x14a/0x240
> >>>>
> >>>> [ 6.898144] hci_le_create_big_complete_evt+0x3d8/0x470
> >>>>
> >>>> [ 6.898146] ? hci_le_remote_feat_complete_evt+0x3e0/0x3e0
> >>>>
> >>>> [ 6.898148] hci_le_meta_evt+0x155/0x230
> >>>>
> >>>> [ 6.898150] hci_event_packet+0x328/0x820
> >>>>
> >>>> [ 6.898152] ? hci_conn_drop+0x100/0x100
> >>>>
> >>>> [ 6.898155] hci_rx_work+0x725/0xb70
> >>>>
> >>>> [ 6.898157] process_one_work+0x2a6/0x5d0
> >>>>
> >>>> [ 6.898160] worker_thread+0x4a/0x3e0
> >>>>
> >>>> [ 6.898162] ? process_one_work+0x5d0/0x5d0
> >>>>
> >>>> [ 6.898164] kthread+0xed/0x120
> >>>>
> >>>> [ 6.898165] ? kthread_complete_and_exit+0x20/0x20
> >>>>
> >>>> [ 6.898167] ret_from_fork+0x22/0x30
> >>>>
> >>>> [ 6.898170] </TASK>
> >>>>
> >>>>
> >>>>
> >>>> I bisected this bug and it was introduced with eca0ae4aea66 ("Bluetooth: Add initial implementation of BIS connections").
> >>>>
> >>>>
> >>>>
> >>>> I am not really sure how to solve that. As far as I understand, previously we simply set an unused handle as connection handle, and checked for that before setting the correct handle and adding it to sysfs. But here, adding it to sysfs seems to happen in a different function and the handle is already set.
> >>>
> >>> We should probably check if link-type, if it is an ISO link it shall
> >>> not be created via Connection Complete events and they have their own
> >>> events to create that.
> I wonder if we can check the connection type in hci_le_create_big_complete_evt()
> and hci_le_cis_estabilished_evt(), as below:
>
> diff --git a/net/bluetooth/hci_event.c b/net/bluetooth/hci_event.c
> index 6643c9c20fa4..5b83473d51b5 100644
> --- a/net/bluetooth/hci_event.c
> +++ b/net/bluetooth/hci_event.c
> @@ -6795,8 +6795,16 @@ static void hci_le_cis_estabilished_evt(struct hci_dev *hdev, void *data,
>
> if (!ev->status) {
> conn->state = BT_CONNECTED;
> - hci_debugfs_create_conn(conn);
> - hci_conn_add_sysfs(conn);
> +
> + /* Only ISO_LINK link type need to register connection device
> + * here, others will register in their relative
> + * Connection Complete events
> + */
> + if (conn->type == ISO_LINK) {
> + hci_debugfs_create_conn(conn);
> + hci_conn_add_sysfs(conn);
> + }
We should probably just bail out if conn->type != ISO_LINK which can
be done much earlier.
> hci_iso_setup_path(conn);
> goto unlock;
> }
> @@ -6901,8 +6909,16 @@ static void hci_le_create_big_complete_evt(struct hci_dev *hdev, void *data,
>
> if (!ev->status) {
> conn->state = BT_CONNECTED;
> - hci_debugfs_create_conn(conn);
> - hci_conn_add_sysfs(conn);
> +
> + /* Only ISO_LINK link type need to register connection device
> + * here, others will register in their relative
> + * Connection Complete events
> + */
> + if (conn->type == ISO_LINK) {
> + hci_debugfs_create_conn(conn);
> + hci_conn_add_sysfs(conn);
> + }
> +
> hci_iso_setup_path(conn);
> goto unlock;
> }
>
> It seems that this patch can pass the syzbot test.
>
> Link: https://lore.kernel.org/all/0000000000004f9ca105e8ba8157@google.com/
> Reported-and-tested-by: syzbot+5a2d2b4a8ca80ad216a9@syzkaller.appspotmail.com
>
> Link: https://lore.kernel.org/all/0000000000008a7a3f05e8ad02f6@google.com/
> Reported-and-tested-by: syzbot+e653e3f67251b6139aaa@syzkaller.appspotmail.com
>
> >>>
> >>
> >>But then the problem of duplicate hci_le_cis_estabilished_evt / hci_le_create_big_complete_evt still exists, isn't it? For example if the connection is created through a hci_le_cis_req_evt, and afterwards two hci_le_cis_estabilished_evt arrive, the second event calls hci_conn_add_sysfs a second time which frees parts of the device structure.
> As for this problem, I wonder if we can check the connection state in those
> functions as below, liking patch
> d5ebaa7c5f6f("Bluetooth: hci_event: Ignore multiple conn complete events"):
>
> diff --git a/net/bluetooth/hci_event.c b/net/bluetooth/hci_event.c
> index 5b83473d51b5..f6b62cfcf082 100644
> --- a/net/bluetooth/hci_event.c
> +++ b/net/bluetooth/hci_event.c
> @@ -6794,6 +6794,14 @@ static void hci_le_cis_estabilished_evt(struct hci_dev *hdev, void *data,
> }
>
> if (!ev->status) {
> + /* The HCI_LE_CIS_Estabilished event is only sent once per connection.
> + * Processing it more than once per connection can corrupt kernel memory.
> + *
> + * As the connection state is set here for the first time, it indicates
> + * whether the connection is already set up.
> + */
> + if (conn->state == BT_CONNECTED)
> + goto unlock;
> conn->state = BT_CONNECTED;
>
> /* Only ISO_LINK link type need to register connection device
> @@ -6908,6 +6916,14 @@ static void hci_le_create_big_complete_evt(struct hci_dev *hdev, void *data,
> conn->handle = __le16_to_cpu(ev->bis_handle[0]);
>
> if (!ev->status) {
> + /* The HCI_LE_Create_BIG_Complete event is only sent once per connection.
> + * Processing it more than once per connection can corrupt kernel memory.
> + *
> + * As the connection state is set here for the first time, it indicates
> + * whether the connection is already set up.
> + */
> + if (conn->state == BT_CONNECTED)
> + goto unlock;
These changes look good, please send a proper patch.
> conn->state = BT_CONNECTED;
>
> /* Only ISO_LINK link type need to register connection device
>
> >>
> >>>> Best
> >>>> Sönke
> >I wonder that if we need both two patches? Because they
> >seems to be used to patch different bugs?
--
Luiz Augusto von Dentz
^ permalink raw reply [flat|nested] 8+ messages in thread
* Re: [syzbot] BUG: corrupted list in kobject_add_internal (4)
2022-09-19 16:55 ` Luiz Augusto von Dentz
@ 2022-09-20 5:10 ` Hawkins Jiawei
2022-09-20 5:23 ` Luiz Augusto von Dentz
2022-09-20 5:49 ` Hawkins Jiawei
0 siblings, 2 replies; 8+ messages in thread
From: Hawkins Jiawei @ 2022-09-20 5:10 UTC (permalink / raw)
To: luiz.dentz
Cc: 18801353760, gregkh, linux-bluetooth, linux-kernel, marcel,
rafael, soenke.huster, syzbot+5a2d2b4a8ca80ad216a9,
syzbot+e653e3f67251b6139aaa, syzkaller-bugs, yin31149
Hi Luiz,
On Tue, 20 Sept 2022 at 00:55, Luiz Augusto von Dentz <luiz.dentz@gmail.com> wrote:
>
> Hi Hawkins,
>
> On Mon, Sep 19, 2022 at 1:42 AM Hawkins Jiawei <yin31149@gmail.com> wrote:
> >
> > On Sat, 17 Sept 2022 at 09:47, Hawkins Jiawei <yin31149@gmail.com> wrote:
> > >
> > >Hi,
> > >
> > >On Fri, 26 Aug 2022 08:27:06, AM Sönke Huster <soenke.huster@eknoes.de> wrote:
> > >>Hi Luiz,
> > >>
> > >>On 25.08.22 20:58, Luiz Augusto von Dentz wrote:
> > >>> Hi Sönke,
> > >>>
> > >>> On Thu, Aug 25, 2022 at 8:08 AM Sönke Huster <soenke.huster@eknoes.de> wrote:
> > >>>>
> > >>>> Hi,
> > >>>>
> > >>>>
> > >>>>
> > >>>> While fuzzing I found several crashes similar to the following:
> > >>>>
> > >>>>
> > >>>> [ 5.345731] sysfs: cannot create duplicate filename '/devices/virtual/bluetooth/hci0/hci0:0'
> > >>>>
> > >>>> [...]
> > >>>>
> > >>>> [ 5.430464] BUG: KASAN: use-after-free in klist_add_tail+0x1bd/0x200
> > >>>>
> > >>>> [ 5.430464] Write of size 8 at addr ffff88800bfcc468 by task kworker/u3:1/43
> > >>>>
> > >>>> [ 5.430464]
> > >>>>
> > >>>> [ 5.430464] CPU: 0 PID: 43 Comm: kworker/u3:1 Not tainted 5.19.0-12855-g13f222680b8f #2
> > >>>>
> > >>>> [ 5.430464] Hardware name: QEMU Standard PC (i440FX + PIIX, 1996), BIOS rel-1.16.0-0-gd239552ce722-prebuilt.qemu.org 04/01/2014
> > >>>>
> > >>>> [ 5.430464] Workqueue: hci0 hci_rx_work
> > >>>>
> > >>>> [ 5.430464] Call Trace:
> > >>>>
> > >>>> [ 5.430464] <TASK>
> > >>>>
> > >>>> [ 5.430464] dump_stack_lvl+0x45/0x5d
> > >>>>
> > >>>> [ 5.430464] print_report.cold+0x5e/0x5e5
> > >>>>
> > >>>> [ 5.430464] kasan_report+0xb1/0x1c0
> > >>>>
> > >>>> [ 5.430464] klist_add_tail+0x1bd/0x200
> > >>>>
> > >>>> [ 5.430464] device_add+0xa6b/0x1b80
> > >>>>
> > >>>> [ 5.430464] hci_conn_add_sysfs+0x91/0x110
> > >>>>
> > >>>> [ 5.430464] le_conn_complete_evt+0x117f/0x17d0
> > >>>>
> > >>>> [ 5.430464] hci_le_conn_complete_evt+0x226/0x2c0
> > >>>>
> > >>>> [ 5.430464] hci_le_meta_evt+0x2c0/0x4a0
> > >>>>
> > >>>> [ 5.430464] hci_event_packet+0x636/0xf60
> > >>>>
> > >>>> [ 5.430464] hci_rx_work+0xa8c/0x1000
> > >>>>
> > >>>> [ 5.430464] process_one_work+0x8df/0x1530
> > >>>>
> > >>>> [ 5.430464] worker_thread+0x575/0x11a0
> > >>>>
> > >>>> [ 5.430464] kthread+0x29d/0x340
> > >>>>
> > >>>> [ 5.430464] ret_from_fork+0x22/0x30
> > >>>>
> > >>>> [ 5.430464] </TASK>
> > >>>>
> > >>>> [ 5.430464]
> > >>>>
> > >>>> [ 5.430464] Allocated by task 44:
> > >>>>
> > >>>> [ 5.430464] kasan_save_stack+0x1e/0x40
> > >>>>
> > >>>> [ 5.430464] __kasan_kmalloc+0x81/0xa0
> > >>>>
> > >>>> [ 5.430464] device_add+0xcae/0x1b80
> > >>>>
> > >>>> [ 5.430464] hci_conn_add_sysfs+0x91/0x110
> > >>>>
> > >>>> [ 5.430464] le_conn_complete_evt+0x117f/0x17d0
> > >>>>
> > >>>> [ 5.430464] hci_le_conn_complete_evt+0x226/0x2c0
> > >>>>
> > >>>> [ 5.430464] hci_le_meta_evt+0x2c0/0x4a0
> > >>>>
> > >>>> [ 5.430464] hci_event_packet+0x636/0xf60
> > >>>>
> > >>>> [ 5.430464] hci_rx_work+0xa8c/0x1000
> > >>>>
> > >>>> [ 5.430464] process_one_work+0x8df/0x1530
> > >>>>
> > >>>> [ 5.430464] worker_thread+0x575/0x11a0
> > >>>>
> > >>>> [ 5.430464] kthread+0x29d/0x340
> > >>>>
> > >>>> [ 5.430464] ret_from_fork+0x22/0x30
> > >>>>
> > >>>> [ 5.430464]
> > >>>>
> > >>>> [ 5.430464] Freed by task 43:
> > >>>>
> > >>>> [ 5.430464] kasan_save_stack+0x1e/0x40
> > >>>>
> > >>>> [ 5.430464] kasan_set_track+0x21/0x30
> > >>>>
> > >>>> [ 5.430464] kasan_set_free_info+0x20/0x40
> > >>>>
> > >>>> [ 5.430464] __kasan_slab_free+0x108/0x190
> > >>>>
> > >>>> [ 5.430464] kfree+0xa9/0x360
> > >>>>
> > >>>> [ 5.430464] device_add+0x33a/0x1b80
> > >>>>
> > >>>> [ 5.430464] hci_conn_add_sysfs+0x91/0x110
> > >>>>
> > >>>> [ 5.430464] hci_le_cis_estabilished_evt+0x517/0xa70
> > >>>>
> > >>>> [ 5.430464] hci_le_meta_evt+0x2c0/0x4a0
> > >>>>
> > >>>> [ 5.430464] hci_event_packet+0x636/0xf60
> > >>>>
> > >>>> [ 5.430464] hci_rx_work+0xa8c/0x1000
> > >>>>
> > >>>> [ 5.430464] process_one_work+0x8df/0x1530
> > >>>>
> > >>>> [ 5.430464] worker_thread+0x575/0x11a0
> > >>>>
> > >>>> [ 5.430464] kthread+0x29d/0x340
> > >>>>
> > >>>> [ 5.430464] ret_from_fork+0x22/0x30
> > >>>>
> > >>>>
> > >>>>
> > >>>> I think I fixed a similar issue in d5ebaa7c5f6f ("Bluetooth: hci_event: Ignore multiple conn complete events"). Here, the problem was that multiple connection complete events for the same handle called hci_conn_add_sysfs multiple times, but if it is called with an existing connection conn->dev->p is freed.
> > >>>>
> > >>>> This is because device_add is called - its documentation contains this sentence: "Do not call this routine or device_register() more than once for any device structure".
> > >>>>
> > >>>>
> > >>>>
> > >>>> This here is similar: First, an hci_le_conn_complete_evt creates a new connection.
> > >>>>
> > >>>> Afterwards, an hci_le_cis_estabilished_evt with the same handle finds that connection, and tries to add it to sysfs again, freeing conn->dev->p. Now, an event that might use that connection such as here the hci_le_conn_complete_evt triggers a use after free.
> > >>>>
> > >
> > >Syzkaller reports a bug as follows [1]:
> > >------------[ cut here ]------------
> > >kernel BUG at lib/list_debug.c:33!
> > >invalid opcode: 0000 [#1] PREEMPT SMP KASAN
> > >[...]
> > >Call Trace:
> > > <TASK>
> > > __list_add include/linux/list.h:69 [inline]
> > > list_add_tail include/linux/list.h:102 [inline]
> > > kobj_kset_join lib/kobject.c:164 [inline]
> > > kobject_add_internal+0x18f/0x8f0 lib/kobject.c:214
> > > kobject_add_varg lib/kobject.c:358 [inline]
> > > kobject_add+0x150/0x1c0 lib/kobject.c:410
> > > device_add+0x368/0x1e90 drivers/base/core.c:3452
> > > hci_conn_add_sysfs+0x9b/0x1b0 net/bluetooth/hci_sysfs.c:53
> > > hci_le_cis_estabilished_evt+0x57c/0xae0 net/bluetooth/hci_event.c:6799
> > > hci_le_meta_evt+0x2b8/0x510 net/bluetooth/hci_event.c:7110
> > > hci_event_func net/bluetooth/hci_event.c:7440 [inline]
> > > hci_event_packet+0x63d/0xfd0 net/bluetooth/hci_event.c:7495
> > > hci_rx_work+0xae7/0x1230 net/bluetooth/hci_core.c:4007
> > > process_one_work+0x991/0x1610 kernel/workqueue.c:2289
> > > worker_thread+0x665/0x1080 kernel/workqueue.c:2436
> > > kthread+0x2e4/0x3a0 kernel/kthread.c:376
> > > ret_from_fork+0x1f/0x30 arch/x86/entry/entry_64.S:306
> > > </TASK>
> > >
> > >I think they are the same problems:
> > >A hci_le_conn_complete_evt creates a new connection, and calls
> > >hci_conn_add_sysfs(). Then hci_le_cis_estabilished_evt with the same handle
> > >finds that connection, and will also calls hci_conn_add_sysfs(), which maybe
> > >triggering corrupted list bug.
> > >
> > >Link: https://syzkaller.appspot.com/bug?id=da3246e2d33afdb92d66bc166a0934c5b146404a [1]
> > >
> > >>>>
> > >>>>
> > >>>> I bisected this bug and it was introduced with 26afbd826ee3 ("Bluetooth: Add initial implementation of CIS connections").
> > >>>>
> > >>>>
> > >>>>
> > >>>> The same happens with hci_le_create_big_complete_evt: Here, multiple events trigger the following bug:
> > >>>>
> > >>>>
> > >>>>
> > >>>> [ 6.898080] BUG: kernel NULL pointer dereference, address: 0000000000000058
> > >>>>
> > >>>> [ 6.898081] #PF: supervisor read access in kernel mode
> > >>>>
> > >>>> [ 6.898083] #PF: error_code(0x0000) - not-present page
> > >>>>
> > >>>> [ 6.898085] Oops: 0000 [#1] PREEMPT SMP NOPTI
> > >>>>
> > >>>> [ 6.898090] Workqueue: hci0 hci_rx_work
> > >>>>
> > >>>> [ 6.898092] RIP: 0010:klist_next+0x12/0x160
> > >>>>
> > >>>> [ 6.898128] Call Trace:
> > >>>>
> > >>>> [ 6.898129] <TASK>
> > >>>>
> > >>>> [ 6.898130] ? bt_link_release+0x20/0x20
> > >>>>
> > >>>> [ 6.898133] device_find_child+0x37/0xa0
> > >>>>
> > >>>> [ 6.898136] hci_conn_del_sysfs+0x71/0xa0
> > >>>>
> > >>>> [ 6.898138] hci_conn_cleanup+0x17a/0x2c0
> > >>>>
> > >>>> [ 6.898141] hci_conn_del+0x14a/0x240
> > >>>>
> > >>>> [ 6.898144] hci_le_create_big_complete_evt+0x3d8/0x470
> > >>>>
> > >>>> [ 6.898146] ? hci_le_remote_feat_complete_evt+0x3e0/0x3e0
> > >>>>
> > >>>> [ 6.898148] hci_le_meta_evt+0x155/0x230
> > >>>>
> > >>>> [ 6.898150] hci_event_packet+0x328/0x820
> > >>>>
> > >>>> [ 6.898152] ? hci_conn_drop+0x100/0x100
> > >>>>
> > >>>> [ 6.898155] hci_rx_work+0x725/0xb70
> > >>>>
> > >>>> [ 6.898157] process_one_work+0x2a6/0x5d0
> > >>>>
> > >>>> [ 6.898160] worker_thread+0x4a/0x3e0
> > >>>>
> > >>>> [ 6.898162] ? process_one_work+0x5d0/0x5d0
> > >>>>
> > >>>> [ 6.898164] kthread+0xed/0x120
> > >>>>
> > >>>> [ 6.898165] ? kthread_complete_and_exit+0x20/0x20
> > >>>>
> > >>>> [ 6.898167] ret_from_fork+0x22/0x30
> > >>>>
> > >>>> [ 6.898170] </TASK>
> > >>>>
> > >>>>
> > >>>>
> > >>>> I bisected this bug and it was introduced with eca0ae4aea66 ("Bluetooth: Add initial implementation of BIS connections").
> > >>>>
> > >>>>
> > >>>>
> > >>>> I am not really sure how to solve that. As far as I understand, previously we simply set an unused handle as connection handle, and checked for that before setting the correct handle and adding it to sysfs. But here, adding it to sysfs seems to happen in a different function and the handle is already set.
> > >>>
> > >>> We should probably check if link-type, if it is an ISO link it shall
> > >>> not be created via Connection Complete events and they have their own
> > >>> events to create that.
> > I wonder if we can check the connection type in hci_le_create_big_complete_evt()
> > and hci_le_cis_estabilished_evt(), as below:
> >
> > diff --git a/net/bluetooth/hci_event.c b/net/bluetooth/hci_event.c
> > index 6643c9c20fa4..5b83473d51b5 100644
> > --- a/net/bluetooth/hci_event.c
> > +++ b/net/bluetooth/hci_event.c
> > @@ -6795,8 +6795,16 @@ static void hci_le_cis_estabilished_evt(struct hci_dev *hdev, void *data,
> >
> > if (!ev->status) {
> > conn->state = BT_CONNECTED;
> > - hci_debugfs_create_conn(conn);
> > - hci_conn_add_sysfs(conn);
> > +
> > + /* Only ISO_LINK link type need to register connection device
> > + * here, others will register in their relative
> > + * Connection Complete events
> > + */
> > + if (conn->type == ISO_LINK) {
> > + hci_debugfs_create_conn(conn);
> > + hci_conn_add_sysfs(conn);
> > + }
>
> We should probably just bail out if conn->type != ISO_LINK which can
> be done much earlier.
Thanks for explanation.
>
> > hci_iso_setup_path(conn);
> > goto unlock;
> > }
> > @@ -6901,8 +6909,16 @@ static void hci_le_create_big_complete_evt(struct hci_dev *hdev, void *data,
> >
> > if (!ev->status) {
> > conn->state = BT_CONNECTED;
> > - hci_debugfs_create_conn(conn);
> > - hci_conn_add_sysfs(conn);
> > +
> > + /* Only ISO_LINK link type need to register connection device
> > + * here, others will register in their relative
> > + * Connection Complete events
> > + */
> > + if (conn->type == ISO_LINK) {
> > + hci_debugfs_create_conn(conn);
> > + hci_conn_add_sysfs(conn);
> > + }
> > +
> > hci_iso_setup_path(conn);
> > goto unlock;
> > }
> >
> > It seems that this patch can pass the syzbot test.
> >
> > Link: https://lore.kernel.org/all/0000000000004f9ca105e8ba8157@google.com/
> > Reported-and-tested-by: syzbot+5a2d2b4a8ca80ad216a9@syzkaller.appspotmail.com
> >
> > Link: https://lore.kernel.org/all/0000000000008a7a3f05e8ad02f6@google.com/
> > Reported-and-tested-by: syzbot+e653e3f67251b6139aaa@syzkaller.appspotmail.com
> >
> > >>>
> > >>
> > >>But then the problem of duplicate hci_le_cis_estabilished_evt / hci_le_create_big_complete_evt still exists, isn't it? For example if the connection is created through a hci_le_cis_req_evt, and afterwards two hci_le_cis_estabilished_evt arrive, the second event calls hci_conn_add_sysfs a second time which frees parts of the device structure.
> > As for this problem, I wonder if we can check the connection state in those
> > functions as below, liking patch
> > d5ebaa7c5f6f("Bluetooth: hci_event: Ignore multiple conn complete events"):
> >
> > diff --git a/net/bluetooth/hci_event.c b/net/bluetooth/hci_event.c
> > index 5b83473d51b5..f6b62cfcf082 100644
> > --- a/net/bluetooth/hci_event.c
> > +++ b/net/bluetooth/hci_event.c
> > @@ -6794,6 +6794,14 @@ static void hci_le_cis_estabilished_evt(struct hci_dev *hdev, void *data,
> > }
> >
> > if (!ev->status) {
> > + /* The HCI_LE_CIS_Estabilished event is only sent once per connection.
> > + * Processing it more than once per connection can corrupt kernel memory.
> > + *
> > + * As the connection state is set here for the first time, it indicates
> > + * whether the connection is already set up.
> > + */
> > + if (conn->state == BT_CONNECTED)
> > + goto unlock;
> > conn->state = BT_CONNECTED;
> >
> > /* Only ISO_LINK link type need to register connection device
> > @@ -6908,6 +6916,14 @@ static void hci_le_create_big_complete_evt(struct hci_dev *hdev, void *data,
> > conn->handle = __le16_to_cpu(ev->bis_handle[0]);
> >
> > if (!ev->status) {
> > + /* The HCI_LE_Create_BIG_Complete event is only sent once per connection.
> > + * Processing it more than once per connection can corrupt kernel memory.
> > + *
> > + * As the connection state is set here for the first time, it indicates
> > + * whether the connection is already set up.
> > + */
> > + if (conn->state == BT_CONNECTED)
> > + goto unlock;
>
> These changes look good, please send a proper patch.
OK, I will add some error message and send a proper patch.
>
> > conn->state = BT_CONNECTED;
> >
> > /* Only ISO_LINK link type need to register connection device
> >
> > >>
> > >>>> Best
> > >>>> Sönke
> > >I wonder that if we need both two patches? Because they
> > >seems to be used to patch different bugs?
>
>
>
> --
> Luiz Augusto von Dentz
^ permalink raw reply [flat|nested] 8+ messages in thread
* Re: [syzbot] BUG: corrupted list in kobject_add_internal (4)
2022-09-20 5:10 ` Hawkins Jiawei
@ 2022-09-20 5:23 ` Luiz Augusto von Dentz
2022-09-20 5:49 ` Hawkins Jiawei
1 sibling, 0 replies; 8+ messages in thread
From: Luiz Augusto von Dentz @ 2022-09-20 5:23 UTC (permalink / raw)
To: Hawkins Jiawei
Cc: 18801353760, gregkh, linux-bluetooth, linux-kernel, marcel,
rafael, soenke.huster, syzbot+5a2d2b4a8ca80ad216a9,
syzbot+e653e3f67251b6139aaa, syzkaller-bugs
Hi Hawkins,
On Mon, Sep 19, 2022 at 10:12 PM Hawkins Jiawei <yin31149@gmail.com> wrote:
>
> Hi Luiz,
>
> On Tue, 20 Sept 2022 at 00:55, Luiz Augusto von Dentz <luiz.dentz@gmail.com> wrote:
> >
> > Hi Hawkins,
> >
> > On Mon, Sep 19, 2022 at 1:42 AM Hawkins Jiawei <yin31149@gmail.com> wrote:
> > >
> > > On Sat, 17 Sept 2022 at 09:47, Hawkins Jiawei <yin31149@gmail.com> wrote:
> > > >
> > > >Hi,
> > > >
> > > >On Fri, 26 Aug 2022 08:27:06, AM Sönke Huster <soenke.huster@eknoes.de> wrote:
> > > >>Hi Luiz,
> > > >>
> > > >>On 25.08.22 20:58, Luiz Augusto von Dentz wrote:
> > > >>> Hi Sönke,
> > > >>>
> > > >>> On Thu, Aug 25, 2022 at 8:08 AM Sönke Huster <soenke.huster@eknoes.de> wrote:
> > > >>>>
> > > >>>> Hi,
> > > >>>>
> > > >>>>
> > > >>>>
> > > >>>> While fuzzing I found several crashes similar to the following:
> > > >>>>
> > > >>>>
> > > >>>> [ 5.345731] sysfs: cannot create duplicate filename '/devices/virtual/bluetooth/hci0/hci0:0'
> > > >>>>
> > > >>>> [...]
> > > >>>>
> > > >>>> [ 5.430464] BUG: KASAN: use-after-free in klist_add_tail+0x1bd/0x200
> > > >>>>
> > > >>>> [ 5.430464] Write of size 8 at addr ffff88800bfcc468 by task kworker/u3:1/43
> > > >>>>
> > > >>>> [ 5.430464]
> > > >>>>
> > > >>>> [ 5.430464] CPU: 0 PID: 43 Comm: kworker/u3:1 Not tainted 5.19.0-12855-g13f222680b8f #2
> > > >>>>
> > > >>>> [ 5.430464] Hardware name: QEMU Standard PC (i440FX + PIIX, 1996), BIOS rel-1.16.0-0-gd239552ce722-prebuilt.qemu.org 04/01/2014
> > > >>>>
> > > >>>> [ 5.430464] Workqueue: hci0 hci_rx_work
> > > >>>>
> > > >>>> [ 5.430464] Call Trace:
> > > >>>>
> > > >>>> [ 5.430464] <TASK>
> > > >>>>
> > > >>>> [ 5.430464] dump_stack_lvl+0x45/0x5d
> > > >>>>
> > > >>>> [ 5.430464] print_report.cold+0x5e/0x5e5
> > > >>>>
> > > >>>> [ 5.430464] kasan_report+0xb1/0x1c0
> > > >>>>
> > > >>>> [ 5.430464] klist_add_tail+0x1bd/0x200
> > > >>>>
> > > >>>> [ 5.430464] device_add+0xa6b/0x1b80
> > > >>>>
> > > >>>> [ 5.430464] hci_conn_add_sysfs+0x91/0x110
> > > >>>>
> > > >>>> [ 5.430464] le_conn_complete_evt+0x117f/0x17d0
> > > >>>>
> > > >>>> [ 5.430464] hci_le_conn_complete_evt+0x226/0x2c0
> > > >>>>
> > > >>>> [ 5.430464] hci_le_meta_evt+0x2c0/0x4a0
> > > >>>>
> > > >>>> [ 5.430464] hci_event_packet+0x636/0xf60
> > > >>>>
> > > >>>> [ 5.430464] hci_rx_work+0xa8c/0x1000
> > > >>>>
> > > >>>> [ 5.430464] process_one_work+0x8df/0x1530
> > > >>>>
> > > >>>> [ 5.430464] worker_thread+0x575/0x11a0
> > > >>>>
> > > >>>> [ 5.430464] kthread+0x29d/0x340
> > > >>>>
> > > >>>> [ 5.430464] ret_from_fork+0x22/0x30
> > > >>>>
> > > >>>> [ 5.430464] </TASK>
> > > >>>>
> > > >>>> [ 5.430464]
> > > >>>>
> > > >>>> [ 5.430464] Allocated by task 44:
> > > >>>>
> > > >>>> [ 5.430464] kasan_save_stack+0x1e/0x40
> > > >>>>
> > > >>>> [ 5.430464] __kasan_kmalloc+0x81/0xa0
> > > >>>>
> > > >>>> [ 5.430464] device_add+0xcae/0x1b80
> > > >>>>
> > > >>>> [ 5.430464] hci_conn_add_sysfs+0x91/0x110
> > > >>>>
> > > >>>> [ 5.430464] le_conn_complete_evt+0x117f/0x17d0
> > > >>>>
> > > >>>> [ 5.430464] hci_le_conn_complete_evt+0x226/0x2c0
> > > >>>>
> > > >>>> [ 5.430464] hci_le_meta_evt+0x2c0/0x4a0
> > > >>>>
> > > >>>> [ 5.430464] hci_event_packet+0x636/0xf60
> > > >>>>
> > > >>>> [ 5.430464] hci_rx_work+0xa8c/0x1000
> > > >>>>
> > > >>>> [ 5.430464] process_one_work+0x8df/0x1530
> > > >>>>
> > > >>>> [ 5.430464] worker_thread+0x575/0x11a0
> > > >>>>
> > > >>>> [ 5.430464] kthread+0x29d/0x340
> > > >>>>
> > > >>>> [ 5.430464] ret_from_fork+0x22/0x30
> > > >>>>
> > > >>>> [ 5.430464]
> > > >>>>
> > > >>>> [ 5.430464] Freed by task 43:
> > > >>>>
> > > >>>> [ 5.430464] kasan_save_stack+0x1e/0x40
> > > >>>>
> > > >>>> [ 5.430464] kasan_set_track+0x21/0x30
> > > >>>>
> > > >>>> [ 5.430464] kasan_set_free_info+0x20/0x40
> > > >>>>
> > > >>>> [ 5.430464] __kasan_slab_free+0x108/0x190
> > > >>>>
> > > >>>> [ 5.430464] kfree+0xa9/0x360
> > > >>>>
> > > >>>> [ 5.430464] device_add+0x33a/0x1b80
> > > >>>>
> > > >>>> [ 5.430464] hci_conn_add_sysfs+0x91/0x110
> > > >>>>
> > > >>>> [ 5.430464] hci_le_cis_estabilished_evt+0x517/0xa70
> > > >>>>
> > > >>>> [ 5.430464] hci_le_meta_evt+0x2c0/0x4a0
> > > >>>>
> > > >>>> [ 5.430464] hci_event_packet+0x636/0xf60
> > > >>>>
> > > >>>> [ 5.430464] hci_rx_work+0xa8c/0x1000
> > > >>>>
> > > >>>> [ 5.430464] process_one_work+0x8df/0x1530
> > > >>>>
> > > >>>> [ 5.430464] worker_thread+0x575/0x11a0
> > > >>>>
> > > >>>> [ 5.430464] kthread+0x29d/0x340
> > > >>>>
> > > >>>> [ 5.430464] ret_from_fork+0x22/0x30
> > > >>>>
> > > >>>>
> > > >>>>
> > > >>>> I think I fixed a similar issue in d5ebaa7c5f6f ("Bluetooth: hci_event: Ignore multiple conn complete events"). Here, the problem was that multiple connection complete events for the same handle called hci_conn_add_sysfs multiple times, but if it is called with an existing connection conn->dev->p is freed.
> > > >>>>
> > > >>>> This is because device_add is called - its documentation contains this sentence: "Do not call this routine or device_register() more than once for any device structure".
> > > >>>>
> > > >>>>
> > > >>>>
> > > >>>> This here is similar: First, an hci_le_conn_complete_evt creates a new connection.
> > > >>>>
> > > >>>> Afterwards, an hci_le_cis_estabilished_evt with the same handle finds that connection, and tries to add it to sysfs again, freeing conn->dev->p. Now, an event that might use that connection such as here the hci_le_conn_complete_evt triggers a use after free.
> > > >>>>
> > > >
> > > >Syzkaller reports a bug as follows [1]:
> > > >------------[ cut here ]------------
> > > >kernel BUG at lib/list_debug.c:33!
> > > >invalid opcode: 0000 [#1] PREEMPT SMP KASAN
> > > >[...]
> > > >Call Trace:
> > > > <TASK>
> > > > __list_add include/linux/list.h:69 [inline]
> > > > list_add_tail include/linux/list.h:102 [inline]
> > > > kobj_kset_join lib/kobject.c:164 [inline]
> > > > kobject_add_internal+0x18f/0x8f0 lib/kobject.c:214
> > > > kobject_add_varg lib/kobject.c:358 [inline]
> > > > kobject_add+0x150/0x1c0 lib/kobject.c:410
> > > > device_add+0x368/0x1e90 drivers/base/core.c:3452
> > > > hci_conn_add_sysfs+0x9b/0x1b0 net/bluetooth/hci_sysfs.c:53
> > > > hci_le_cis_estabilished_evt+0x57c/0xae0 net/bluetooth/hci_event.c:6799
> > > > hci_le_meta_evt+0x2b8/0x510 net/bluetooth/hci_event.c:7110
> > > > hci_event_func net/bluetooth/hci_event.c:7440 [inline]
> > > > hci_event_packet+0x63d/0xfd0 net/bluetooth/hci_event.c:7495
> > > > hci_rx_work+0xae7/0x1230 net/bluetooth/hci_core.c:4007
> > > > process_one_work+0x991/0x1610 kernel/workqueue.c:2289
> > > > worker_thread+0x665/0x1080 kernel/workqueue.c:2436
> > > > kthread+0x2e4/0x3a0 kernel/kthread.c:376
> > > > ret_from_fork+0x1f/0x30 arch/x86/entry/entry_64.S:306
> > > > </TASK>
> > > >
> > > >I think they are the same problems:
> > > >A hci_le_conn_complete_evt creates a new connection, and calls
> > > >hci_conn_add_sysfs(). Then hci_le_cis_estabilished_evt with the same handle
> > > >finds that connection, and will also calls hci_conn_add_sysfs(), which maybe
> > > >triggering corrupted list bug.
> > > >
> > > >Link: https://syzkaller.appspot.com/bug?id=da3246e2d33afdb92d66bc166a0934c5b146404a [1]
> > > >
> > > >>>>
> > > >>>>
> > > >>>> I bisected this bug and it was introduced with 26afbd826ee3 ("Bluetooth: Add initial implementation of CIS connections").
> > > >>>>
> > > >>>>
> > > >>>>
> > > >>>> The same happens with hci_le_create_big_complete_evt: Here, multiple events trigger the following bug:
> > > >>>>
> > > >>>>
> > > >>>>
> > > >>>> [ 6.898080] BUG: kernel NULL pointer dereference, address: 0000000000000058
> > > >>>>
> > > >>>> [ 6.898081] #PF: supervisor read access in kernel mode
> > > >>>>
> > > >>>> [ 6.898083] #PF: error_code(0x0000) - not-present page
> > > >>>>
> > > >>>> [ 6.898085] Oops: 0000 [#1] PREEMPT SMP NOPTI
> > > >>>>
> > > >>>> [ 6.898090] Workqueue: hci0 hci_rx_work
> > > >>>>
> > > >>>> [ 6.898092] RIP: 0010:klist_next+0x12/0x160
> > > >>>>
> > > >>>> [ 6.898128] Call Trace:
> > > >>>>
> > > >>>> [ 6.898129] <TASK>
> > > >>>>
> > > >>>> [ 6.898130] ? bt_link_release+0x20/0x20
> > > >>>>
> > > >>>> [ 6.898133] device_find_child+0x37/0xa0
> > > >>>>
> > > >>>> [ 6.898136] hci_conn_del_sysfs+0x71/0xa0
> > > >>>>
> > > >>>> [ 6.898138] hci_conn_cleanup+0x17a/0x2c0
> > > >>>>
> > > >>>> [ 6.898141] hci_conn_del+0x14a/0x240
> > > >>>>
> > > >>>> [ 6.898144] hci_le_create_big_complete_evt+0x3d8/0x470
> > > >>>>
> > > >>>> [ 6.898146] ? hci_le_remote_feat_complete_evt+0x3e0/0x3e0
> > > >>>>
> > > >>>> [ 6.898148] hci_le_meta_evt+0x155/0x230
> > > >>>>
> > > >>>> [ 6.898150] hci_event_packet+0x328/0x820
> > > >>>>
> > > >>>> [ 6.898152] ? hci_conn_drop+0x100/0x100
> > > >>>>
> > > >>>> [ 6.898155] hci_rx_work+0x725/0xb70
> > > >>>>
> > > >>>> [ 6.898157] process_one_work+0x2a6/0x5d0
> > > >>>>
> > > >>>> [ 6.898160] worker_thread+0x4a/0x3e0
> > > >>>>
> > > >>>> [ 6.898162] ? process_one_work+0x5d0/0x5d0
> > > >>>>
> > > >>>> [ 6.898164] kthread+0xed/0x120
> > > >>>>
> > > >>>> [ 6.898165] ? kthread_complete_and_exit+0x20/0x20
> > > >>>>
> > > >>>> [ 6.898167] ret_from_fork+0x22/0x30
> > > >>>>
> > > >>>> [ 6.898170] </TASK>
> > > >>>>
> > > >>>>
> > > >>>>
> > > >>>> I bisected this bug and it was introduced with eca0ae4aea66 ("Bluetooth: Add initial implementation of BIS connections").
> > > >>>>
> > > >>>>
> > > >>>>
> > > >>>> I am not really sure how to solve that. As far as I understand, previously we simply set an unused handle as connection handle, and checked for that before setting the correct handle and adding it to sysfs. But here, adding it to sysfs seems to happen in a different function and the handle is already set.
> > > >>>
> > > >>> We should probably check if link-type, if it is an ISO link it shall
> > > >>> not be created via Connection Complete events and they have their own
> > > >>> events to create that.
> > > I wonder if we can check the connection type in hci_le_create_big_complete_evt()
> > > and hci_le_cis_estabilished_evt(), as below:
> > >
> > > diff --git a/net/bluetooth/hci_event.c b/net/bluetooth/hci_event.c
> > > index 6643c9c20fa4..5b83473d51b5 100644
> > > --- a/net/bluetooth/hci_event.c
> > > +++ b/net/bluetooth/hci_event.c
> > > @@ -6795,8 +6795,16 @@ static void hci_le_cis_estabilished_evt(struct hci_dev *hdev, void *data,
> > >
> > > if (!ev->status) {
> > > conn->state = BT_CONNECTED;
> > > - hci_debugfs_create_conn(conn);
> > > - hci_conn_add_sysfs(conn);
> > > +
> > > + /* Only ISO_LINK link type need to register connection device
> > > + * here, others will register in their relative
> > > + * Connection Complete events
> > > + */
> > > + if (conn->type == ISO_LINK) {
> > > + hci_debugfs_create_conn(conn);
> > > + hci_conn_add_sysfs(conn);
> > > + }
> >
> > We should probably just bail out if conn->type != ISO_LINK which can
> > be done much earlier.
> Thanks for explanation.
https://patchwork.kernel.org/project/bluetooth/patch/20220919181017.1658995-1-luiz.dentz@gmail.com/
> >
> > > hci_iso_setup_path(conn);
> > > goto unlock;
> > > }
> > > @@ -6901,8 +6909,16 @@ static void hci_le_create_big_complete_evt(struct hci_dev *hdev, void *data,
> > >
> > > if (!ev->status) {
> > > conn->state = BT_CONNECTED;
> > > - hci_debugfs_create_conn(conn);
> > > - hci_conn_add_sysfs(conn);
> > > +
> > > + /* Only ISO_LINK link type need to register connection device
> > > + * here, others will register in their relative
> > > + * Connection Complete events
> > > + */
> > > + if (conn->type == ISO_LINK) {
> > > + hci_debugfs_create_conn(conn);
> > > + hci_conn_add_sysfs(conn);
> > > + }
> > > +
> > > hci_iso_setup_path(conn);
> > > goto unlock;
> > > }
> > >
> > > It seems that this patch can pass the syzbot test.
> > >
> > > Link: https://lore.kernel.org/all/0000000000004f9ca105e8ba8157@google.com/
> > > Reported-and-tested-by: syzbot+5a2d2b4a8ca80ad216a9@syzkaller.appspotmail.com
> > >
> > > Link: https://lore.kernel.org/all/0000000000008a7a3f05e8ad02f6@google.com/
> > > Reported-and-tested-by: syzbot+e653e3f67251b6139aaa@syzkaller.appspotmail.com
> > >
> > > >>>
> > > >>
> > > >>But then the problem of duplicate hci_le_cis_estabilished_evt / hci_le_create_big_complete_evt still exists, isn't it? For example if the connection is created through a hci_le_cis_req_evt, and afterwards two hci_le_cis_estabilished_evt arrive, the second event calls hci_conn_add_sysfs a second time which frees parts of the device structure.
> > > As for this problem, I wonder if we can check the connection state in those
> > > functions as below, liking patch
> > > d5ebaa7c5f6f("Bluetooth: hci_event: Ignore multiple conn complete events"):
> > >
> > > diff --git a/net/bluetooth/hci_event.c b/net/bluetooth/hci_event.c
> > > index 5b83473d51b5..f6b62cfcf082 100644
> > > --- a/net/bluetooth/hci_event.c
> > > +++ b/net/bluetooth/hci_event.c
> > > @@ -6794,6 +6794,14 @@ static void hci_le_cis_estabilished_evt(struct hci_dev *hdev, void *data,
> > > }
> > >
> > > if (!ev->status) {
> > > + /* The HCI_LE_CIS_Estabilished event is only sent once per connection.
> > > + * Processing it more than once per connection can corrupt kernel memory.
> > > + *
> > > + * As the connection state is set here for the first time, it indicates
> > > + * whether the connection is already set up.
> > > + */
> > > + if (conn->state == BT_CONNECTED)
> > > + goto unlock;
> > > conn->state = BT_CONNECTED;
> > >
> > > /* Only ISO_LINK link type need to register connection device
> > > @@ -6908,6 +6916,14 @@ static void hci_le_create_big_complete_evt(struct hci_dev *hdev, void *data,
> > > conn->handle = __le16_to_cpu(ev->bis_handle[0]);
> > >
> > > if (!ev->status) {
> > > + /* The HCI_LE_Create_BIG_Complete event is only sent once per connection.
> > > + * Processing it more than once per connection can corrupt kernel memory.
> > > + *
> > > + * As the connection state is set here for the first time, it indicates
> > > + * whether the connection is already set up.
> > > + */
> > > + if (conn->state == BT_CONNECTED)
> > > + goto unlock;
> >
> > These changes look good, please send a proper patch.
> OK, I will add some error message and send a proper patch.
Note that I did send a set that should resolve this as well:
https://patchwork.kernel.org/project/bluetooth/list/?series=678331
> >
> > > conn->state = BT_CONNECTED;
> > >
> > > /* Only ISO_LINK link type need to register connection device
> > >
> > > >>
> > > >>>> Best
> > > >>>> Sönke
> > > >I wonder that if we need both two patches? Because they
> > > >seems to be used to patch different bugs?
> >
> >
> >
> > --
> > Luiz Augusto von Dentz
--
Luiz Augusto von Dentz
^ permalink raw reply [flat|nested] 8+ messages in thread
* Re: [syzbot] BUG: corrupted list in kobject_add_internal (4)
2022-09-20 5:10 ` Hawkins Jiawei
2022-09-20 5:23 ` Luiz Augusto von Dentz
@ 2022-09-20 5:49 ` Hawkins Jiawei
2022-09-20 20:23 ` Luiz Augusto von Dentz
1 sibling, 1 reply; 8+ messages in thread
From: Hawkins Jiawei @ 2022-09-20 5:49 UTC (permalink / raw)
To: yin31149
Cc: 18801353760, gregkh, linux-bluetooth, linux-kernel, luiz.dentz,
marcel, rafael, soenke.huster, syzbot+5a2d2b4a8ca80ad216a9,
syzbot+e653e3f67251b6139aaa, syzkaller-bugs
Hi Luiz,
On Tue, 20 Sept 2022 at 13:23, Luiz Augusto von Dentz <luiz.dentz@gmail.com> wrote:
>
> Hi Hawkins,
>
> On Mon, Sep 19, 2022 at 10:12 PM Hawkins Jiawei <yin31149@gmail.com> wrote:
> >
> > Hi Luiz,
> >
> > On Tue, 20 Sept 2022 at 00:55, Luiz Augusto von Dentz <luiz.dentz@gmail.com> wrote:
> > >
> > > Hi Hawkins,
> > >
> > > On Mon, Sep 19, 2022 at 1:42 AM Hawkins Jiawei <yin31149@gmail.com> wrote:
> > > >
> > > > On Sat, 17 Sept 2022 at 09:47, Hawkins Jiawei <yin31149@gmail.com> wrote:
> > > > >
> > > > >Hi,
> > > > >
> > > > >On Fri, 26 Aug 2022 08:27:06, AM Sönke Huster <soenke.huster@eknoes.de> wrote:
> > > > >>Hi Luiz,
> > > > >>
> > > > >>On 25.08.22 20:58, Luiz Augusto von Dentz wrote:
> > > > >>> Hi Sönke,
> > > > >>>
> > > > >>> On Thu, Aug 25, 2022 at 8:08 AM Sönke Huster <soenke.huster@eknoes.de> wrote:
> > > > >>>>
> > > > >>>> Hi,
> > > > >>>>
> > > > >>>>
> > > > >>>>
> > > > >>>> While fuzzing I found several crashes similar to the following:
> > > > >>>>
> > > > >>>>
> > > > >>>> [ 5.345731] sysfs: cannot create duplicate filename '/devices/virtual/bluetooth/hci0/hci0:0'
> > > > >>>>
> > > > >>>> [...]
> > > > >>>>
> > > > >>>> [ 5.430464] BUG: KASAN: use-after-free in klist_add_tail+0x1bd/0x200
> > > > >>>>
> > > > >>>> [ 5.430464] Write of size 8 at addr ffff88800bfcc468 by task kworker/u3:1/43
> > > > >>>>
> > > > >>>> [ 5.430464]
> > > > >>>>
> > > > >>>> [ 5.430464] CPU: 0 PID: 43 Comm: kworker/u3:1 Not tainted 5.19.0-12855-g13f222680b8f #2
> > > > >>>>
> > > > >>>> [ 5.430464] Hardware name: QEMU Standard PC (i440FX + PIIX, 1996), BIOS rel-1.16.0-0-gd239552ce722-prebuilt.qemu.org 04/01/2014
> > > > >>>>
> > > > >>>> [ 5.430464] Workqueue: hci0 hci_rx_work
> > > > >>>>
> > > > >>>> [ 5.430464] Call Trace:
> > > > >>>>
> > > > >>>> [ 5.430464] <TASK>
> > > > >>>>
> > > > >>>> [ 5.430464] dump_stack_lvl+0x45/0x5d
> > > > >>>>
> > > > >>>> [ 5.430464] print_report.cold+0x5e/0x5e5
> > > > >>>>
> > > > >>>> [ 5.430464] kasan_report+0xb1/0x1c0
> > > > >>>>
> > > > >>>> [ 5.430464] klist_add_tail+0x1bd/0x200
> > > > >>>>
> > > > >>>> [ 5.430464] device_add+0xa6b/0x1b80
> > > > >>>>
> > > > >>>> [ 5.430464] hci_conn_add_sysfs+0x91/0x110
> > > > >>>>
> > > > >>>> [ 5.430464] le_conn_complete_evt+0x117f/0x17d0
> > > > >>>>
> > > > >>>> [ 5.430464] hci_le_conn_complete_evt+0x226/0x2c0
> > > > >>>>
> > > > >>>> [ 5.430464] hci_le_meta_evt+0x2c0/0x4a0
> > > > >>>>
> > > > >>>> [ 5.430464] hci_event_packet+0x636/0xf60
> > > > >>>>
> > > > >>>> [ 5.430464] hci_rx_work+0xa8c/0x1000
> > > > >>>>
> > > > >>>> [ 5.430464] process_one_work+0x8df/0x1530
> > > > >>>>
> > > > >>>> [ 5.430464] worker_thread+0x575/0x11a0
> > > > >>>>
> > > > >>>> [ 5.430464] kthread+0x29d/0x340
> > > > >>>>
> > > > >>>> [ 5.430464] ret_from_fork+0x22/0x30
> > > > >>>>
> > > > >>>> [ 5.430464] </TASK>
> > > > >>>>
> > > > >>>> [ 5.430464]
> > > > >>>>
> > > > >>>> [ 5.430464] Allocated by task 44:
> > > > >>>>
> > > > >>>> [ 5.430464] kasan_save_stack+0x1e/0x40
> > > > >>>>
> > > > >>>> [ 5.430464] __kasan_kmalloc+0x81/0xa0
> > > > >>>>
> > > > >>>> [ 5.430464] device_add+0xcae/0x1b80
> > > > >>>>
> > > > >>>> [ 5.430464] hci_conn_add_sysfs+0x91/0x110
> > > > >>>>
> > > > >>>> [ 5.430464] le_conn_complete_evt+0x117f/0x17d0
> > > > >>>>
> > > > >>>> [ 5.430464] hci_le_conn_complete_evt+0x226/0x2c0
> > > > >>>>
> > > > >>>> [ 5.430464] hci_le_meta_evt+0x2c0/0x4a0
> > > > >>>>
> > > > >>>> [ 5.430464] hci_event_packet+0x636/0xf60
> > > > >>>>
> > > > >>>> [ 5.430464] hci_rx_work+0xa8c/0x1000
> > > > >>>>
> > > > >>>> [ 5.430464] process_one_work+0x8df/0x1530
> > > > >>>>
> > > > >>>> [ 5.430464] worker_thread+0x575/0x11a0
> > > > >>>>
> > > > >>>> [ 5.430464] kthread+0x29d/0x340
> > > > >>>>
> > > > >>>> [ 5.430464] ret_from_fork+0x22/0x30
> > > > >>>>
> > > > >>>> [ 5.430464]
> > > > >>>>
> > > > >>>> [ 5.430464] Freed by task 43:
> > > > >>>>
> > > > >>>> [ 5.430464] kasan_save_stack+0x1e/0x40
> > > > >>>>
> > > > >>>> [ 5.430464] kasan_set_track+0x21/0x30
> > > > >>>>
> > > > >>>> [ 5.430464] kasan_set_free_info+0x20/0x40
> > > > >>>>
> > > > >>>> [ 5.430464] __kasan_slab_free+0x108/0x190
> > > > >>>>
> > > > >>>> [ 5.430464] kfree+0xa9/0x360
> > > > >>>>
> > > > >>>> [ 5.430464] device_add+0x33a/0x1b80
> > > > >>>>
> > > > >>>> [ 5.430464] hci_conn_add_sysfs+0x91/0x110
> > > > >>>>
> > > > >>>> [ 5.430464] hci_le_cis_estabilished_evt+0x517/0xa70
> > > > >>>>
> > > > >>>> [ 5.430464] hci_le_meta_evt+0x2c0/0x4a0
> > > > >>>>
> > > > >>>> [ 5.430464] hci_event_packet+0x636/0xf60
> > > > >>>>
> > > > >>>> [ 5.430464] hci_rx_work+0xa8c/0x1000
> > > > >>>>
> > > > >>>> [ 5.430464] process_one_work+0x8df/0x1530
> > > > >>>>
> > > > >>>> [ 5.430464] worker_thread+0x575/0x11a0
> > > > >>>>
> > > > >>>> [ 5.430464] kthread+0x29d/0x340
> > > > >>>>
> > > > >>>> [ 5.430464] ret_from_fork+0x22/0x30
> > > > >>>>
> > > > >>>>
> > > > >>>>
> > > > >>>> I think I fixed a similar issue in d5ebaa7c5f6f ("Bluetooth: hci_event: Ignore multiple conn complete events"). Here, the problem was that multiple connection complete events for the same handle called hci_conn_add_sysfs multiple times, but if it is called with an existing connection conn->dev->p is freed.
> > > > >>>>
> > > > >>>> This is because device_add is called - its documentation contains this sentence: "Do not call this routine or device_register() more than once for any device structure".
> > > > >>>>
> > > > >>>>
> > > > >>>>
> > > > >>>> This here is similar: First, an hci_le_conn_complete_evt creates a new connection.
> > > > >>>>
> > > > >>>> Afterwards, an hci_le_cis_estabilished_evt with the same handle finds that connection, and tries to add it to sysfs again, freeing conn->dev->p. Now, an event that might use that connection such as here the hci_le_conn_complete_evt triggers a use after free.
> > > > >>>>
> > > > >
> > > > >Syzkaller reports a bug as follows [1]:
> > > > >------------[ cut here ]------------
> > > > >kernel BUG at lib/list_debug.c:33!
> > > > >invalid opcode: 0000 [#1] PREEMPT SMP KASAN
> > > > >[...]
> > > > >Call Trace:
> > > > > <TASK>
> > > > > __list_add include/linux/list.h:69 [inline]
> > > > > list_add_tail include/linux/list.h:102 [inline]
> > > > > kobj_kset_join lib/kobject.c:164 [inline]
> > > > > kobject_add_internal+0x18f/0x8f0 lib/kobject.c:214
> > > > > kobject_add_varg lib/kobject.c:358 [inline]
> > > > > kobject_add+0x150/0x1c0 lib/kobject.c:410
> > > > > device_add+0x368/0x1e90 drivers/base/core.c:3452
> > > > > hci_conn_add_sysfs+0x9b/0x1b0 net/bluetooth/hci_sysfs.c:53
> > > > > hci_le_cis_estabilished_evt+0x57c/0xae0 net/bluetooth/hci_event.c:6799
> > > > > hci_le_meta_evt+0x2b8/0x510 net/bluetooth/hci_event.c:7110
> > > > > hci_event_func net/bluetooth/hci_event.c:7440 [inline]
> > > > > hci_event_packet+0x63d/0xfd0 net/bluetooth/hci_event.c:7495
> > > > > hci_rx_work+0xae7/0x1230 net/bluetooth/hci_core.c:4007
> > > > > process_one_work+0x991/0x1610 kernel/workqueue.c:2289
> > > > > worker_thread+0x665/0x1080 kernel/workqueue.c:2436
> > > > > kthread+0x2e4/0x3a0 kernel/kthread.c:376
> > > > > ret_from_fork+0x1f/0x30 arch/x86/entry/entry_64.S:306
> > > > > </TASK>
> > > > >
> > > > >I think they are the same problems:
> > > > >A hci_le_conn_complete_evt creates a new connection, and calls
> > > > >hci_conn_add_sysfs(). Then hci_le_cis_estabilished_evt with the same handle
> > > > >finds that connection, and will also calls hci_conn_add_sysfs(), which maybe
> > > > >triggering corrupted list bug.
> > > > >
> > > > >Link: https://syzkaller.appspot.com/bug?id=da3246e2d33afdb92d66bc166a0934c5b146404a [1]
> > > > >
> > > > >>>>
> > > > >>>>
> > > > >>>> I bisected this bug and it was introduced with 26afbd826ee3 ("Bluetooth: Add initial implementation of CIS connections").
> > > > >>>>
> > > > >>>>
> > > > >>>>
> > > > >>>> The same happens with hci_le_create_big_complete_evt: Here, multiple events trigger the following bug:
> > > > >>>>
> > > > >>>>
> > > > >>>>
> > > > >>>> [ 6.898080] BUG: kernel NULL pointer dereference, address: 0000000000000058
> > > > >>>>
> > > > >>>> [ 6.898081] #PF: supervisor read access in kernel mode
> > > > >>>>
> > > > >>>> [ 6.898083] #PF: error_code(0x0000) - not-present page
> > > > >>>>
> > > > >>>> [ 6.898085] Oops: 0000 [#1] PREEMPT SMP NOPTI
> > > > >>>>
> > > > >>>> [ 6.898090] Workqueue: hci0 hci_rx_work
> > > > >>>>
> > > > >>>> [ 6.898092] RIP: 0010:klist_next+0x12/0x160
> > > > >>>>
> > > > >>>> [ 6.898128] Call Trace:
> > > > >>>>
> > > > >>>> [ 6.898129] <TASK>
> > > > >>>>
> > > > >>>> [ 6.898130] ? bt_link_release+0x20/0x20
> > > > >>>>
> > > > >>>> [ 6.898133] device_find_child+0x37/0xa0
> > > > >>>>
> > > > >>>> [ 6.898136] hci_conn_del_sysfs+0x71/0xa0
> > > > >>>>
> > > > >>>> [ 6.898138] hci_conn_cleanup+0x17a/0x2c0
> > > > >>>>
> > > > >>>> [ 6.898141] hci_conn_del+0x14a/0x240
> > > > >>>>
> > > > >>>> [ 6.898144] hci_le_create_big_complete_evt+0x3d8/0x470
> > > > >>>>
> > > > >>>> [ 6.898146] ? hci_le_remote_feat_complete_evt+0x3e0/0x3e0
> > > > >>>>
> > > > >>>> [ 6.898148] hci_le_meta_evt+0x155/0x230
> > > > >>>>
> > > > >>>> [ 6.898150] hci_event_packet+0x328/0x820
> > > > >>>>
> > > > >>>> [ 6.898152] ? hci_conn_drop+0x100/0x100
> > > > >>>>
> > > > >>>> [ 6.898155] hci_rx_work+0x725/0xb70
> > > > >>>>
> > > > >>>> [ 6.898157] process_one_work+0x2a6/0x5d0
> > > > >>>>
> > > > >>>> [ 6.898160] worker_thread+0x4a/0x3e0
> > > > >>>>
> > > > >>>> [ 6.898162] ? process_one_work+0x5d0/0x5d0
> > > > >>>>
> > > > >>>> [ 6.898164] kthread+0xed/0x120
> > > > >>>>
> > > > >>>> [ 6.898165] ? kthread_complete_and_exit+0x20/0x20
> > > > >>>>
> > > > >>>> [ 6.898167] ret_from_fork+0x22/0x30
> > > > >>>>
> > > > >>>> [ 6.898170] </TASK>
> > > > >>>>
> > > > >>>>
> > > > >>>>
> > > > >>>> I bisected this bug and it was introduced with eca0ae4aea66 ("Bluetooth: Add initial implementation of BIS connections").
> > > > >>>>
> > > > >>>>
> > > > >>>>
> > > > >>>> I am not really sure how to solve that. As far as I understand, previously we simply set an unused handle as connection handle, and checked for that before setting the correct handle and adding it to sysfs. But here, adding it to sysfs seems to happen in a different function and the handle is already set.
> > > > >>>
> > > > >>> We should probably check if link-type, if it is an ISO link it shall
> > > > >>> not be created via Connection Complete events and they have their own
> > > > >>> events to create that.
> > > > I wonder if we can check the connection type in hci_le_create_big_complete_evt()
> > > > and hci_le_cis_estabilished_evt(), as below:
> > > >
> > > > diff --git a/net/bluetooth/hci_event.c b/net/bluetooth/hci_event.c
> > > > index 6643c9c20fa4..5b83473d51b5 100644
> > > > --- a/net/bluetooth/hci_event.c
> > > > +++ b/net/bluetooth/hci_event.c
> > > > @@ -6795,8 +6795,16 @@ static void hci_le_cis_estabilished_evt(struct hci_dev *hdev, void *data,
> > > >
> > > > if (!ev->status) {
> > > > conn->state = BT_CONNECTED;
> > > > - hci_debugfs_create_conn(conn);
> > > > - hci_conn_add_sysfs(conn);
> > > > +
> > > > + /* Only ISO_LINK link type need to register connection device
> > > > + * here, others will register in their relative
> > > > + * Connection Complete events
> > > > + */
> > > > + if (conn->type == ISO_LINK) {
> > > > + hci_debugfs_create_conn(conn);
> > > > + hci_conn_add_sysfs(conn);
> > > > + }
> > >
> > > We should probably just bail out if conn->type != ISO_LINK which can
> > > be done much earlier.
> > Thanks for explanation.
> https://patchwork.kernel.org/project/bluetooth/patch/20220919181017.1658995-1-luiz.dentz@gmail.com/
Thanks for link.
>
> > >
> > > > hci_iso_setup_path(conn);
> > > > goto unlock;
> > > > }
> > > > @@ -6901,8 +6909,16 @@ static void hci_le_create_big_complete_evt(struct hci_dev *hdev, void *data,
> > > >
> > > > if (!ev->status) {
> > > > conn->state = BT_CONNECTED;
> > > > - hci_debugfs_create_conn(conn);
> > > > - hci_conn_add_sysfs(conn);
> > > > +
> > > > + /* Only ISO_LINK link type need to register connection device
> > > > + * here, others will register in their relative
> > > > + * Connection Complete events
> > > > + */
> > > > + if (conn->type == ISO_LINK) {
> > > > + hci_debugfs_create_conn(conn);
> > > > + hci_conn_add_sysfs(conn);
> > > > + }
> > > > +
> > > > hci_iso_setup_path(conn);
> > > > goto unlock;
> > > > }
> > > >
> > > > It seems that this patch can pass the syzbot test.
> > > >
> > > > Link: https://lore.kernel.org/all/0000000000004f9ca105e8ba8157@google.com/
> > > > Reported-and-tested-by: syzbot+5a2d2b4a8ca80ad216a9@syzkaller.appspotmail.com
> > > >
> > > > Link: https://lore.kernel.org/all/0000000000008a7a3f05e8ad02f6@google.com/
> > > > Reported-and-tested-by: syzbot+e653e3f67251b6139aaa@syzkaller.appspotmail.com
> > > >
> > > > >>>
> > > > >>
> > > > >>But then the problem of duplicate hci_le_cis_estabilished_evt / hci_le_create_big_complete_evt still exists, isn't it? For example if the connection is created through a hci_le_cis_req_evt, and afterwards two hci_le_cis_estabilished_evt arrive, the second event calls hci_conn_add_sysfs a second time which frees parts of the device structure.
> > > > As for this problem, I wonder if we can check the connection state in those
> > > > functions as below, liking patch
> > > > d5ebaa7c5f6f("Bluetooth: hci_event: Ignore multiple conn complete events"):
> > > >
> > > > diff --git a/net/bluetooth/hci_event.c b/net/bluetooth/hci_event.c
> > > > index 5b83473d51b5..f6b62cfcf082 100644
> > > > --- a/net/bluetooth/hci_event.c
> > > > +++ b/net/bluetooth/hci_event.c
> > > > @@ -6794,6 +6794,14 @@ static void hci_le_cis_estabilished_evt(struct hci_dev *hdev, void *data,
> > > > }
> > > >
> > > > if (!ev->status) {
> > > > + /* The HCI_LE_CIS_Estabilished event is only sent once per connection.
> > > > + * Processing it more than once per connection can corrupt kernel memory.
> > > > + *
> > > > + * As the connection state is set here for the first time, it indicates
> > > > + * whether the connection is already set up.
> > > > + */
> > > > + if (conn->state == BT_CONNECTED)
> > > > + goto unlock;
> > > > conn->state = BT_CONNECTED;
> > > >
> > > > /* Only ISO_LINK link type need to register connection device
> > > > @@ -6908,6 +6916,14 @@ static void hci_le_create_big_complete_evt(struct hci_dev *hdev, void *data,
> > > > conn->handle = __le16_to_cpu(ev->bis_handle[0]);
> > > >
> > > > if (!ev->status) {
> > > > + /* The HCI_LE_Create_BIG_Complete event is only sent once per connection.
> > > > + * Processing it more than once per connection can corrupt kernel memory.
> > > > + *
> > > > + * As the connection state is set here for the first time, it indicates
> > > > + * whether the connection is already set up.
> > > > + */
> > > > + if (conn->state == BT_CONNECTED)
> > > > + goto unlock;
> > >
> > > These changes look good, please send a proper patch.
> > OK, I will add some error message and send a proper patch.
>
> Note that I did send a set that should resolve this as well:
>
> https://patchwork.kernel.org/project/bluetooth/list/?series=678331
>
Right, it seems better to patch this bug in this way.
> > >
> > > > conn->state = BT_CONNECTED;
> > > >
> > > > /* Only ISO_LINK link type need to register connection device
> > > >
> > > > >>
> > > > >>>> Best
> > > > >>>> Sönke
> > > > >I wonder that if we need both two patches? Because they
> > > > >seems to be used to patch different bugs?
> > >
> > >
> > >
> > > --
> > > Luiz Augusto von Dentz
>
>
>
> --
> Luiz Augusto von Dentz
^ permalink raw reply [flat|nested] 8+ messages in thread
* Re: [syzbot] BUG: corrupted list in kobject_add_internal (4)
2022-09-20 5:49 ` Hawkins Jiawei
@ 2022-09-20 20:23 ` Luiz Augusto von Dentz
0 siblings, 0 replies; 8+ messages in thread
From: Luiz Augusto von Dentz @ 2022-09-20 20:23 UTC (permalink / raw)
To: Hawkins Jiawei
Cc: 18801353760, gregkh, linux-bluetooth, linux-kernel, marcel,
rafael, soenke.huster, syzbot+5a2d2b4a8ca80ad216a9,
syzbot+e653e3f67251b6139aaa, syzkaller-bugs
Hi Hawkins,
On Mon, Sep 19, 2022 at 10:49 PM Hawkins Jiawei <yin31149@gmail.com> wrote:
>
> Hi Luiz,
>
> On Tue, 20 Sept 2022 at 13:23, Luiz Augusto von Dentz <luiz.dentz@gmail.com> wrote:
> >
> > Hi Hawkins,
> >
> > On Mon, Sep 19, 2022 at 10:12 PM Hawkins Jiawei <yin31149@gmail.com> wrote:
> > >
> > > Hi Luiz,
> > >
> > > On Tue, 20 Sept 2022 at 00:55, Luiz Augusto von Dentz <luiz.dentz@gmail.com> wrote:
> > > >
> > > > Hi Hawkins,
> > > >
> > > > On Mon, Sep 19, 2022 at 1:42 AM Hawkins Jiawei <yin31149@gmail.com> wrote:
> > > > >
> > > > > On Sat, 17 Sept 2022 at 09:47, Hawkins Jiawei <yin31149@gmail.com> wrote:
> > > > > >
> > > > > >Hi,
> > > > > >
> > > > > >On Fri, 26 Aug 2022 08:27:06, AM Sönke Huster <soenke.huster@eknoes.de> wrote:
> > > > > >>Hi Luiz,
> > > > > >>
> > > > > >>On 25.08.22 20:58, Luiz Augusto von Dentz wrote:
> > > > > >>> Hi Sönke,
> > > > > >>>
> > > > > >>> On Thu, Aug 25, 2022 at 8:08 AM Sönke Huster <soenke.huster@eknoes.de> wrote:
> > > > > >>>>
> > > > > >>>> Hi,
> > > > > >>>>
> > > > > >>>>
> > > > > >>>>
> > > > > >>>> While fuzzing I found several crashes similar to the following:
> > > > > >>>>
> > > > > >>>>
> > > > > >>>> [ 5.345731] sysfs: cannot create duplicate filename '/devices/virtual/bluetooth/hci0/hci0:0'
> > > > > >>>>
> > > > > >>>> [...]
> > > > > >>>>
> > > > > >>>> [ 5.430464] BUG: KASAN: use-after-free in klist_add_tail+0x1bd/0x200
> > > > > >>>>
> > > > > >>>> [ 5.430464] Write of size 8 at addr ffff88800bfcc468 by task kworker/u3:1/43
> > > > > >>>>
> > > > > >>>> [ 5.430464]
> > > > > >>>>
> > > > > >>>> [ 5.430464] CPU: 0 PID: 43 Comm: kworker/u3:1 Not tainted 5.19.0-12855-g13f222680b8f #2
> > > > > >>>>
> > > > > >>>> [ 5.430464] Hardware name: QEMU Standard PC (i440FX + PIIX, 1996), BIOS rel-1.16.0-0-gd239552ce722-prebuilt.qemu.org 04/01/2014
> > > > > >>>>
> > > > > >>>> [ 5.430464] Workqueue: hci0 hci_rx_work
> > > > > >>>>
> > > > > >>>> [ 5.430464] Call Trace:
> > > > > >>>>
> > > > > >>>> [ 5.430464] <TASK>
> > > > > >>>>
> > > > > >>>> [ 5.430464] dump_stack_lvl+0x45/0x5d
> > > > > >>>>
> > > > > >>>> [ 5.430464] print_report.cold+0x5e/0x5e5
> > > > > >>>>
> > > > > >>>> [ 5.430464] kasan_report+0xb1/0x1c0
> > > > > >>>>
> > > > > >>>> [ 5.430464] klist_add_tail+0x1bd/0x200
> > > > > >>>>
> > > > > >>>> [ 5.430464] device_add+0xa6b/0x1b80
> > > > > >>>>
> > > > > >>>> [ 5.430464] hci_conn_add_sysfs+0x91/0x110
> > > > > >>>>
> > > > > >>>> [ 5.430464] le_conn_complete_evt+0x117f/0x17d0
> > > > > >>>>
> > > > > >>>> [ 5.430464] hci_le_conn_complete_evt+0x226/0x2c0
> > > > > >>>>
> > > > > >>>> [ 5.430464] hci_le_meta_evt+0x2c0/0x4a0
> > > > > >>>>
> > > > > >>>> [ 5.430464] hci_event_packet+0x636/0xf60
> > > > > >>>>
> > > > > >>>> [ 5.430464] hci_rx_work+0xa8c/0x1000
> > > > > >>>>
> > > > > >>>> [ 5.430464] process_one_work+0x8df/0x1530
> > > > > >>>>
> > > > > >>>> [ 5.430464] worker_thread+0x575/0x11a0
> > > > > >>>>
> > > > > >>>> [ 5.430464] kthread+0x29d/0x340
> > > > > >>>>
> > > > > >>>> [ 5.430464] ret_from_fork+0x22/0x30
> > > > > >>>>
> > > > > >>>> [ 5.430464] </TASK>
> > > > > >>>>
> > > > > >>>> [ 5.430464]
> > > > > >>>>
> > > > > >>>> [ 5.430464] Allocated by task 44:
> > > > > >>>>
> > > > > >>>> [ 5.430464] kasan_save_stack+0x1e/0x40
> > > > > >>>>
> > > > > >>>> [ 5.430464] __kasan_kmalloc+0x81/0xa0
> > > > > >>>>
> > > > > >>>> [ 5.430464] device_add+0xcae/0x1b80
> > > > > >>>>
> > > > > >>>> [ 5.430464] hci_conn_add_sysfs+0x91/0x110
> > > > > >>>>
> > > > > >>>> [ 5.430464] le_conn_complete_evt+0x117f/0x17d0
> > > > > >>>>
> > > > > >>>> [ 5.430464] hci_le_conn_complete_evt+0x226/0x2c0
> > > > > >>>>
> > > > > >>>> [ 5.430464] hci_le_meta_evt+0x2c0/0x4a0
> > > > > >>>>
> > > > > >>>> [ 5.430464] hci_event_packet+0x636/0xf60
> > > > > >>>>
> > > > > >>>> [ 5.430464] hci_rx_work+0xa8c/0x1000
> > > > > >>>>
> > > > > >>>> [ 5.430464] process_one_work+0x8df/0x1530
> > > > > >>>>
> > > > > >>>> [ 5.430464] worker_thread+0x575/0x11a0
> > > > > >>>>
> > > > > >>>> [ 5.430464] kthread+0x29d/0x340
> > > > > >>>>
> > > > > >>>> [ 5.430464] ret_from_fork+0x22/0x30
> > > > > >>>>
> > > > > >>>> [ 5.430464]
> > > > > >>>>
> > > > > >>>> [ 5.430464] Freed by task 43:
> > > > > >>>>
> > > > > >>>> [ 5.430464] kasan_save_stack+0x1e/0x40
> > > > > >>>>
> > > > > >>>> [ 5.430464] kasan_set_track+0x21/0x30
> > > > > >>>>
> > > > > >>>> [ 5.430464] kasan_set_free_info+0x20/0x40
> > > > > >>>>
> > > > > >>>> [ 5.430464] __kasan_slab_free+0x108/0x190
> > > > > >>>>
> > > > > >>>> [ 5.430464] kfree+0xa9/0x360
> > > > > >>>>
> > > > > >>>> [ 5.430464] device_add+0x33a/0x1b80
> > > > > >>>>
> > > > > >>>> [ 5.430464] hci_conn_add_sysfs+0x91/0x110
> > > > > >>>>
> > > > > >>>> [ 5.430464] hci_le_cis_estabilished_evt+0x517/0xa70
> > > > > >>>>
> > > > > >>>> [ 5.430464] hci_le_meta_evt+0x2c0/0x4a0
> > > > > >>>>
> > > > > >>>> [ 5.430464] hci_event_packet+0x636/0xf60
> > > > > >>>>
> > > > > >>>> [ 5.430464] hci_rx_work+0xa8c/0x1000
> > > > > >>>>
> > > > > >>>> [ 5.430464] process_one_work+0x8df/0x1530
> > > > > >>>>
> > > > > >>>> [ 5.430464] worker_thread+0x575/0x11a0
> > > > > >>>>
> > > > > >>>> [ 5.430464] kthread+0x29d/0x340
> > > > > >>>>
> > > > > >>>> [ 5.430464] ret_from_fork+0x22/0x30
> > > > > >>>>
> > > > > >>>>
> > > > > >>>>
> > > > > >>>> I think I fixed a similar issue in d5ebaa7c5f6f ("Bluetooth: hci_event: Ignore multiple conn complete events"). Here, the problem was that multiple connection complete events for the same handle called hci_conn_add_sysfs multiple times, but if it is called with an existing connection conn->dev->p is freed.
> > > > > >>>>
> > > > > >>>> This is because device_add is called - its documentation contains this sentence: "Do not call this routine or device_register() more than once for any device structure".
> > > > > >>>>
> > > > > >>>>
> > > > > >>>>
> > > > > >>>> This here is similar: First, an hci_le_conn_complete_evt creates a new connection.
> > > > > >>>>
> > > > > >>>> Afterwards, an hci_le_cis_estabilished_evt with the same handle finds that connection, and tries to add it to sysfs again, freeing conn->dev->p. Now, an event that might use that connection such as here the hci_le_conn_complete_evt triggers a use after free.
> > > > > >>>>
> > > > > >
> > > > > >Syzkaller reports a bug as follows [1]:
> > > > > >------------[ cut here ]------------
> > > > > >kernel BUG at lib/list_debug.c:33!
> > > > > >invalid opcode: 0000 [#1] PREEMPT SMP KASAN
> > > > > >[...]
> > > > > >Call Trace:
> > > > > > <TASK>
> > > > > > __list_add include/linux/list.h:69 [inline]
> > > > > > list_add_tail include/linux/list.h:102 [inline]
> > > > > > kobj_kset_join lib/kobject.c:164 [inline]
> > > > > > kobject_add_internal+0x18f/0x8f0 lib/kobject.c:214
> > > > > > kobject_add_varg lib/kobject.c:358 [inline]
> > > > > > kobject_add+0x150/0x1c0 lib/kobject.c:410
> > > > > > device_add+0x368/0x1e90 drivers/base/core.c:3452
> > > > > > hci_conn_add_sysfs+0x9b/0x1b0 net/bluetooth/hci_sysfs.c:53
> > > > > > hci_le_cis_estabilished_evt+0x57c/0xae0 net/bluetooth/hci_event.c:6799
> > > > > > hci_le_meta_evt+0x2b8/0x510 net/bluetooth/hci_event.c:7110
> > > > > > hci_event_func net/bluetooth/hci_event.c:7440 [inline]
> > > > > > hci_event_packet+0x63d/0xfd0 net/bluetooth/hci_event.c:7495
> > > > > > hci_rx_work+0xae7/0x1230 net/bluetooth/hci_core.c:4007
> > > > > > process_one_work+0x991/0x1610 kernel/workqueue.c:2289
> > > > > > worker_thread+0x665/0x1080 kernel/workqueue.c:2436
> > > > > > kthread+0x2e4/0x3a0 kernel/kthread.c:376
> > > > > > ret_from_fork+0x1f/0x30 arch/x86/entry/entry_64.S:306
> > > > > > </TASK>
> > > > > >
> > > > > >I think they are the same problems:
> > > > > >A hci_le_conn_complete_evt creates a new connection, and calls
> > > > > >hci_conn_add_sysfs(). Then hci_le_cis_estabilished_evt with the same handle
> > > > > >finds that connection, and will also calls hci_conn_add_sysfs(), which maybe
> > > > > >triggering corrupted list bug.
> > > > > >
> > > > > >Link: https://syzkaller.appspot.com/bug?id=da3246e2d33afdb92d66bc166a0934c5b146404a [1]
> > > > > >
> > > > > >>>>
> > > > > >>>>
> > > > > >>>> I bisected this bug and it was introduced with 26afbd826ee3 ("Bluetooth: Add initial implementation of CIS connections").
> > > > > >>>>
> > > > > >>>>
> > > > > >>>>
> > > > > >>>> The same happens with hci_le_create_big_complete_evt: Here, multiple events trigger the following bug:
> > > > > >>>>
> > > > > >>>>
> > > > > >>>>
> > > > > >>>> [ 6.898080] BUG: kernel NULL pointer dereference, address: 0000000000000058
> > > > > >>>>
> > > > > >>>> [ 6.898081] #PF: supervisor read access in kernel mode
> > > > > >>>>
> > > > > >>>> [ 6.898083] #PF: error_code(0x0000) - not-present page
> > > > > >>>>
> > > > > >>>> [ 6.898085] Oops: 0000 [#1] PREEMPT SMP NOPTI
> > > > > >>>>
> > > > > >>>> [ 6.898090] Workqueue: hci0 hci_rx_work
> > > > > >>>>
> > > > > >>>> [ 6.898092] RIP: 0010:klist_next+0x12/0x160
> > > > > >>>>
> > > > > >>>> [ 6.898128] Call Trace:
> > > > > >>>>
> > > > > >>>> [ 6.898129] <TASK>
> > > > > >>>>
> > > > > >>>> [ 6.898130] ? bt_link_release+0x20/0x20
> > > > > >>>>
> > > > > >>>> [ 6.898133] device_find_child+0x37/0xa0
> > > > > >>>>
> > > > > >>>> [ 6.898136] hci_conn_del_sysfs+0x71/0xa0
> > > > > >>>>
> > > > > >>>> [ 6.898138] hci_conn_cleanup+0x17a/0x2c0
> > > > > >>>>
> > > > > >>>> [ 6.898141] hci_conn_del+0x14a/0x240
> > > > > >>>>
> > > > > >>>> [ 6.898144] hci_le_create_big_complete_evt+0x3d8/0x470
> > > > > >>>>
> > > > > >>>> [ 6.898146] ? hci_le_remote_feat_complete_evt+0x3e0/0x3e0
> > > > > >>>>
> > > > > >>>> [ 6.898148] hci_le_meta_evt+0x155/0x230
> > > > > >>>>
> > > > > >>>> [ 6.898150] hci_event_packet+0x328/0x820
> > > > > >>>>
> > > > > >>>> [ 6.898152] ? hci_conn_drop+0x100/0x100
> > > > > >>>>
> > > > > >>>> [ 6.898155] hci_rx_work+0x725/0xb70
> > > > > >>>>
> > > > > >>>> [ 6.898157] process_one_work+0x2a6/0x5d0
> > > > > >>>>
> > > > > >>>> [ 6.898160] worker_thread+0x4a/0x3e0
> > > > > >>>>
> > > > > >>>> [ 6.898162] ? process_one_work+0x5d0/0x5d0
> > > > > >>>>
> > > > > >>>> [ 6.898164] kthread+0xed/0x120
> > > > > >>>>
> > > > > >>>> [ 6.898165] ? kthread_complete_and_exit+0x20/0x20
> > > > > >>>>
> > > > > >>>> [ 6.898167] ret_from_fork+0x22/0x30
> > > > > >>>>
> > > > > >>>> [ 6.898170] </TASK>
> > > > > >>>>
> > > > > >>>>
> > > > > >>>>
> > > > > >>>> I bisected this bug and it was introduced with eca0ae4aea66 ("Bluetooth: Add initial implementation of BIS connections").
> > > > > >>>>
> > > > > >>>>
> > > > > >>>>
> > > > > >>>> I am not really sure how to solve that. As far as I understand, previously we simply set an unused handle as connection handle, and checked for that before setting the correct handle and adding it to sysfs. But here, adding it to sysfs seems to happen in a different function and the handle is already set.
> > > > > >>>
> > > > > >>> We should probably check if link-type, if it is an ISO link it shall
> > > > > >>> not be created via Connection Complete events and they have their own
> > > > > >>> events to create that.
> > > > > I wonder if we can check the connection type in hci_le_create_big_complete_evt()
> > > > > and hci_le_cis_estabilished_evt(), as below:
> > > > >
> > > > > diff --git a/net/bluetooth/hci_event.c b/net/bluetooth/hci_event.c
> > > > > index 6643c9c20fa4..5b83473d51b5 100644
> > > > > --- a/net/bluetooth/hci_event.c
> > > > > +++ b/net/bluetooth/hci_event.c
> > > > > @@ -6795,8 +6795,16 @@ static void hci_le_cis_estabilished_evt(struct hci_dev *hdev, void *data,
> > > > >
> > > > > if (!ev->status) {
> > > > > conn->state = BT_CONNECTED;
> > > > > - hci_debugfs_create_conn(conn);
> > > > > - hci_conn_add_sysfs(conn);
> > > > > +
> > > > > + /* Only ISO_LINK link type need to register connection device
> > > > > + * here, others will register in their relative
> > > > > + * Connection Complete events
> > > > > + */
> > > > > + if (conn->type == ISO_LINK) {
> > > > > + hci_debugfs_create_conn(conn);
> > > > > + hci_conn_add_sysfs(conn);
> > > > > + }
> > > >
> > > > We should probably just bail out if conn->type != ISO_LINK which can
> > > > be done much earlier.
> > > Thanks for explanation.
> > https://patchwork.kernel.org/project/bluetooth/patch/20220919181017.1658995-1-luiz.dentz@gmail.com/
> Thanks for link.
>
> >
> > > >
> > > > > hci_iso_setup_path(conn);
> > > > > goto unlock;
> > > > > }
> > > > > @@ -6901,8 +6909,16 @@ static void hci_le_create_big_complete_evt(struct hci_dev *hdev, void *data,
> > > > >
> > > > > if (!ev->status) {
> > > > > conn->state = BT_CONNECTED;
> > > > > - hci_debugfs_create_conn(conn);
> > > > > - hci_conn_add_sysfs(conn);
> > > > > +
> > > > > + /* Only ISO_LINK link type need to register connection device
> > > > > + * here, others will register in their relative
> > > > > + * Connection Complete events
> > > > > + */
> > > > > + if (conn->type == ISO_LINK) {
> > > > > + hci_debugfs_create_conn(conn);
> > > > > + hci_conn_add_sysfs(conn);
> > > > > + }
> > > > > +
> > > > > hci_iso_setup_path(conn);
> > > > > goto unlock;
> > > > > }
> > > > >
> > > > > It seems that this patch can pass the syzbot test.
> > > > >
> > > > > Link: https://lore.kernel.org/all/0000000000004f9ca105e8ba8157@google.com/
> > > > > Reported-and-tested-by: syzbot+5a2d2b4a8ca80ad216a9@syzkaller.appspotmail.com
> > > > >
> > > > > Link: https://lore.kernel.org/all/0000000000008a7a3f05e8ad02f6@google.com/
> > > > > Reported-and-tested-by: syzbot+e653e3f67251b6139aaa@syzkaller.appspotmail.com
> > > > >
> > > > > >>>
> > > > > >>
> > > > > >>But then the problem of duplicate hci_le_cis_estabilished_evt / hci_le_create_big_complete_evt still exists, isn't it? For example if the connection is created through a hci_le_cis_req_evt, and afterwards two hci_le_cis_estabilished_evt arrive, the second event calls hci_conn_add_sysfs a second time which frees parts of the device structure.
> > > > > As for this problem, I wonder if we can check the connection state in those
> > > > > functions as below, liking patch
> > > > > d5ebaa7c5f6f("Bluetooth: hci_event: Ignore multiple conn complete events"):
> > > > >
> > > > > diff --git a/net/bluetooth/hci_event.c b/net/bluetooth/hci_event.c
> > > > > index 5b83473d51b5..f6b62cfcf082 100644
> > > > > --- a/net/bluetooth/hci_event.c
> > > > > +++ b/net/bluetooth/hci_event.c
> > > > > @@ -6794,6 +6794,14 @@ static void hci_le_cis_estabilished_evt(struct hci_dev *hdev, void *data,
> > > > > }
> > > > >
> > > > > if (!ev->status) {
> > > > > + /* The HCI_LE_CIS_Estabilished event is only sent once per connection.
> > > > > + * Processing it more than once per connection can corrupt kernel memory.
> > > > > + *
> > > > > + * As the connection state is set here for the first time, it indicates
> > > > > + * whether the connection is already set up.
> > > > > + */
> > > > > + if (conn->state == BT_CONNECTED)
> > > > > + goto unlock;
> > > > > conn->state = BT_CONNECTED;
> > > > >
> > > > > /* Only ISO_LINK link type need to register connection device
> > > > > @@ -6908,6 +6916,14 @@ static void hci_le_create_big_complete_evt(struct hci_dev *hdev, void *data,
> > > > > conn->handle = __le16_to_cpu(ev->bis_handle[0]);
> > > > >
> > > > > if (!ev->status) {
> > > > > + /* The HCI_LE_Create_BIG_Complete event is only sent once per connection.
> > > > > + * Processing it more than once per connection can corrupt kernel memory.
> > > > > + *
> > > > > + * As the connection state is set here for the first time, it indicates
> > > > > + * whether the connection is already set up.
> > > > > + */
> > > > > + if (conn->state == BT_CONNECTED)
> > > > > + goto unlock;
> > > >
> > > > These changes look good, please send a proper patch.
> > > OK, I will add some error message and send a proper patch.
> >
> > Note that I did send a set that should resolve this as well:
> >
> > https://patchwork.kernel.org/project/bluetooth/list/?series=678331
> >
> Right, it seems better to patch this bug in this way.
Can you test it and reply with Tested-by please?
> > > >
> > > > > conn->state = BT_CONNECTED;
> > > > >
> > > > > /* Only ISO_LINK link type need to register connection device
> > > > >
> > > > > >>
> > > > > >>>> Best
> > > > > >>>> Sönke
> > > > > >I wonder that if we need both two patches? Because they
> > > > > >seems to be used to patch different bugs?
> > > >
> > > >
> > > >
> > > > --
> > > > Luiz Augusto von Dentz
> >
> >
> >
> > --
> > Luiz Augusto von Dentz
--
Luiz Augusto von Dentz
^ permalink raw reply [flat|nested] 8+ messages in thread
end of thread, other threads:[~2022-09-20 20:23 UTC | newest]
Thread overview: 8+ messages (download: mbox.gz / follow: Atom feed)
-- links below jump to the message on this page --
2022-09-15 1:17 [syzbot] BUG: corrupted list in kobject_add_internal (4) syzbot
2022-09-17 1:47 ` Hawkins Jiawei
2022-09-19 8:41 ` Hawkins Jiawei
2022-09-19 16:55 ` Luiz Augusto von Dentz
2022-09-20 5:10 ` Hawkins Jiawei
2022-09-20 5:23 ` Luiz Augusto von Dentz
2022-09-20 5:49 ` Hawkins Jiawei
2022-09-20 20:23 ` Luiz Augusto von Dentz
This is an external index of several public inboxes,
see mirroring instructions on how to clone and mirror
all data and code used by this external index.