| Message ID | 20220216043714.22011-1-linma@zju.edu.cn |
|---|---|
| State | Superseded |
| Headers | show |
| Series | [v1] Bluetooth: fix data races in smp_unregister(), smp_del_chan() | expand |
This is automated email and please do not reply to this email! Dear submitter, Thank you for submitting the patches to the linux bluetooth mailing list. This is a CI test results with your patch series: PW Link:https://patchwork.kernel.org/project/bluetooth/list/?series=614795 ---Test result--- Test Summary: CheckPatch PASS 0.77 seconds GitLint PASS 0.42 seconds SubjectPrefix PASS 0.24 seconds BuildKernel PASS 36.72 seconds BuildKernel32 PASS 31.95 seconds Incremental Build with patchesPASS 43.85 seconds TestRunner: Setup PASS 576.01 seconds TestRunner: l2cap-tester PASS 15.71 seconds TestRunner: bnep-tester PASS 7.06 seconds TestRunner: mgmt-tester PASS 122.73 seconds TestRunner: rfcomm-tester FAIL 8.94 seconds TestRunner: sco-tester PASS 9.15 seconds TestRunner: smp-tester PASS 9.07 seconds TestRunner: userchan-tester PASS 7.41 seconds Details ############################## Test: TestRunner: rfcomm-tester - FAIL - 8.94 seconds Run test-runner with rfcomm-tester Total: 10, Passed: 9 (90.0%), Failed: 1, Not Run: 0 Failed Test Cases Basic RFCOMM Socket Client - Write 32k Success Failed 0.192 seconds --- Regards, Linux Bluetooth
Hi Lin, > Previous commit e04480920d1e ("Bluetooth: defer cleanup of resources > in hci_unregister_dev()") defers all destructive actions to > hci_release_dev() to prevent cocurrent problems like NPD, UAF. > > However, there are still some exceptions that are ignored. > > The smp_unregister() in hci_dev_close_sync() (previously in > hci_dev_do_close) will release resources like the sensitive channel > and the smp_dev objects. Consider the situations the device is detaching > or power down while the kernel is still operating on it, the following > data race could take place. > > thread-A hci_dev_close_sync | thread-B read_local_oob_ext_data > | > hci_dev_unlock() | > ... | hci_dev_lock() > if (hdev->smp_data) | > chan = hdev->smp_data | > | chan = hdev->smp_data (3) > | > hdev->smp_data = NULL (1) | if (!chan || !chan->data) (4) > ... | > smp = chan->data | smp = chan->data > if (smp) | > chan->data = NULL (2) | > ... | > kfree_sensitive(smp) | > | // dereference smp trigger UFA > > That is, the objects hdev->smp_data and chan->data both suffer from the > data races. In a preempt-enable kernel, the above schedule (when (3) is > before (1) and (4) is before (2)) leads to UAF bugs. It can be > reproduced in the latest kernel and below is part of the report: > > [ 49.097146] ================================================================ > [ 49.097611] BUG: KASAN: use-after-free in smp_generate_oob+0x2dd/0x570 > [ 49.097611] Read of size 8 at addr ffff888006528360 by task generate_oob/155 > [ 49.097611] > [ 49.097611] Call Trace: > [ 49.097611] <TASK> > [ 49.097611] dump_stack_lvl+0x34/0x44 > [ 49.097611] print_address_description.constprop.0+0x1f/0x150 > [ 49.097611] ? smp_generate_oob+0x2dd/0x570 > [ 49.097611] ? smp_generate_oob+0x2dd/0x570 > [ 49.097611] kasan_report.cold+0x7f/0x11b > [ 49.097611] ? smp_generate_oob+0x2dd/0x570 > [ 49.097611] smp_generate_oob+0x2dd/0x570 > [ 49.097611] read_local_oob_ext_data+0x689/0xc30 > [ 49.097611] ? hci_event_packet+0xc80/0xc80 > [ 49.097611] ? sysvec_apic_timer_interrupt+0x9b/0xc0 > [ 49.097611] ? asm_sysvec_apic_timer_interrupt+0x12/0x20 > [ 49.097611] ? mgmt_init_hdev+0x1c/0x240 > [ 49.097611] ? mgmt_init_hdev+0x28/0x240 > [ 49.097611] hci_sock_sendmsg+0x1880/0x1e70 > [ 49.097611] ? create_monitor_event+0x890/0x890 > [ 49.097611] ? create_monitor_event+0x890/0x890 > [ 49.097611] sock_sendmsg+0xdf/0x110 > [ 49.097611] __sys_sendto+0x19e/0x270 > [ 49.097611] ? __ia32_sys_getpeername+0xa0/0xa0 > [ 49.097611] ? kernel_fpu_begin_mask+0x1c0/0x1c0 > [ 49.097611] __x64_sys_sendto+0xd8/0x1b0 > [ 49.097611] ? syscall_exit_to_user_mode+0x1d/0x40 > [ 49.097611] do_syscall_64+0x3b/0x90 > [ 49.097611] entry_SYSCALL_64_after_hwframe+0x44/0xae > [ 49.097611] RIP: 0033:0x7f5a59f51f64 > ... > [ 49.097611] RAX: ffffffffffffffda RBX: 0000000000000000 RCX: 00007f5a59f51f64 > [ 49.097611] RDX: 0000000000000007 RSI: 00007f5a59d6ac70 RDI: 0000000000000006 > [ 49.097611] RBP: 0000000000000000 R08: 0000000000000000 R09: 0000000000000000 > [ 49.097611] R10: 0000000000000040 R11: 0000000000000246 R12: 00007ffec26916ee > [ 49.097611] R13: 00007ffec26916ef R14: 00007f5a59d6afc0 R15: 00007f5a59d6b700 > > To solve these data races, this patch places the smp_unregister() > function in the protected area by the hci_dev_lock(). That is, the > smp_unregister() function can not be concurrently executed when > operating functions (most of them are mgmt operations in mgmt.c) hold > the device lock. > > This patch is tested with kernel LOCK DEBUGGING enabled. The price from > the extended holding time of the device lock is supposed to be low as the > smp_unregister() function is fairly short and efficient. > > Signed-off-by: Lin Ma <linma@zju.edu.cn> > --- > net/bluetooth/hci_sync.c | 4 ++-- > 1 file changed, 2 insertions(+), 2 deletions(-) patch has been applied to bluetooth-next tree. Regards Marcel
Hi Marcel, Thanks for the timely reply. Regards Lin > -----Original Messages----- > From: "Marcel Holtmann" <marcel@holtmann.org> > Sent Time: 2022-02-16 18:23:52 (Wednesday) > To: "Lin Ma" <linma@zju.edu.cn> > Cc: "Johan Hedberg" <johan.hedberg@gmail.com>, "Luiz Augusto von Dentz" <luiz.dentz@gmail.com>, "David S. Miller" <davem@davemloft.net>, linux-bluetooth@vger.kernel.org > Subject: Re: [PATCH v1] Bluetooth: fix data races in smp_unregister(), smp_del_chan() > > Hi Lin, > > > Previous commit e04480920d1e ("Bluetooth: defer cleanup of resources > > in hci_unregister_dev()") defers all destructive actions to > > hci_release_dev() to prevent cocurrent problems like NPD, UAF. > > > > However, there are still some exceptions that are ignored. > > > > The smp_unregister() in hci_dev_close_sync() (previously in > > hci_dev_do_close) will release resources like the sensitive channel > > and the smp_dev objects. Consider the situations the device is detaching > > or power down while the kernel is still operating on it, the following > > data race could take place. > > > > thread-A hci_dev_close_sync | thread-B read_local_oob_ext_data > > | > > hci_dev_unlock() | > > ... | hci_dev_lock() > > if (hdev->smp_data) | > > chan = hdev->smp_data | > > | chan = hdev->smp_data (3) > > | > > hdev->smp_data = NULL (1) | if (!chan || !chan->data) (4) > > ... | > > smp = chan->data | smp = chan->data > > if (smp) | > > chan->data = NULL (2) | > > ... | > > kfree_sensitive(smp) | > > | // dereference smp trigger UFA > > > > That is, the objects hdev->smp_data and chan->data both suffer from the > > data races. In a preempt-enable kernel, the above schedule (when (3) is > > before (1) and (4) is before (2)) leads to UAF bugs. It can be > > reproduced in the latest kernel and below is part of the report: > > > > [ 49.097146] ================================================================ > > [ 49.097611] BUG: KASAN: use-after-free in smp_generate_oob+0x2dd/0x570 > > [ 49.097611] Read of size 8 at addr ffff888006528360 by task generate_oob/155 > > [ 49.097611] > > [ 49.097611] Call Trace: > > [ 49.097611] <TASK> > > [ 49.097611] dump_stack_lvl+0x34/0x44 > > [ 49.097611] print_address_description.constprop.0+0x1f/0x150 > > [ 49.097611] ? smp_generate_oob+0x2dd/0x570 > > [ 49.097611] ? smp_generate_oob+0x2dd/0x570 > > [ 49.097611] kasan_report.cold+0x7f/0x11b > > [ 49.097611] ? smp_generate_oob+0x2dd/0x570 > > [ 49.097611] smp_generate_oob+0x2dd/0x570 > > [ 49.097611] read_local_oob_ext_data+0x689/0xc30 > > [ 49.097611] ? hci_event_packet+0xc80/0xc80 > > [ 49.097611] ? sysvec_apic_timer_interrupt+0x9b/0xc0 > > [ 49.097611] ? asm_sysvec_apic_timer_interrupt+0x12/0x20 > > [ 49.097611] ? mgmt_init_hdev+0x1c/0x240 > > [ 49.097611] ? mgmt_init_hdev+0x28/0x240 > > [ 49.097611] hci_sock_sendmsg+0x1880/0x1e70 > > [ 49.097611] ? create_monitor_event+0x890/0x890 > > [ 49.097611] ? create_monitor_event+0x890/0x890 > > [ 49.097611] sock_sendmsg+0xdf/0x110 > > [ 49.097611] __sys_sendto+0x19e/0x270 > > [ 49.097611] ? __ia32_sys_getpeername+0xa0/0xa0 > > [ 49.097611] ? kernel_fpu_begin_mask+0x1c0/0x1c0 > > [ 49.097611] __x64_sys_sendto+0xd8/0x1b0 > > [ 49.097611] ? syscall_exit_to_user_mode+0x1d/0x40 > > [ 49.097611] do_syscall_64+0x3b/0x90 > > [ 49.097611] entry_SYSCALL_64_after_hwframe+0x44/0xae > > [ 49.097611] RIP: 0033:0x7f5a59f51f64 > > ... > > [ 49.097611] RAX: ffffffffffffffda RBX: 0000000000000000 RCX: 00007f5a59f51f64 > > [ 49.097611] RDX: 0000000000000007 RSI: 00007f5a59d6ac70 RDI: 0000000000000006 > > [ 49.097611] RBP: 0000000000000000 R08: 0000000000000000 R09: 0000000000000000 > > [ 49.097611] R10: 0000000000000040 R11: 0000000000000246 R12: 00007ffec26916ee > > [ 49.097611] R13: 00007ffec26916ef R14: 00007f5a59d6afc0 R15: 00007f5a59d6b700 > > > > To solve these data races, this patch places the smp_unregister() > > function in the protected area by the hci_dev_lock(). That is, the > > smp_unregister() function can not be concurrently executed when > > operating functions (most of them are mgmt operations in mgmt.c) hold > > the device lock. > > > > This patch is tested with kernel LOCK DEBUGGING enabled. The price from > > the extended holding time of the device lock is supposed to be low as the > > smp_unregister() function is fairly short and efficient. > > > > Signed-off-by: Lin Ma <linma@zju.edu.cn> > > --- > > net/bluetooth/hci_sync.c | 4 ++-- > > 1 file changed, 2 insertions(+), 2 deletions(-) > > patch has been applied to bluetooth-next tree. > > Regards > > Marcel
diff --git a/net/bluetooth/hci_sync.c b/net/bluetooth/hci_sync.c index 0feb68f12545..e34fc15b7d2c 100644 --- a/net/bluetooth/hci_sync.c +++ b/net/bluetooth/hci_sync.c @@ -4106,9 +4106,9 @@ int hci_dev_close_sync(struct hci_dev *hdev) hci_inquiry_cache_flush(hdev); hci_pend_le_actions_clear(hdev); hci_conn_hash_flush(hdev); - hci_dev_unlock(hdev); - + /* Prevent data races on hdev->smp_data or hdev->smp_bredr_data */ smp_unregister(hdev); + hci_dev_unlock(hdev); hci_sock_dev_event(hdev, HCI_DEV_DOWN);
Previous commit e04480920d1e ("Bluetooth: defer cleanup of resources in hci_unregister_dev()") defers all destructive actions to hci_release_dev() to prevent cocurrent problems like NPD, UAF. However, there are still some exceptions that are ignored. The smp_unregister() in hci_dev_close_sync() (previously in hci_dev_do_close) will release resources like the sensitive channel and the smp_dev objects. Consider the situations the device is detaching or power down while the kernel is still operating on it, the following data race could take place. thread-A hci_dev_close_sync | thread-B read_local_oob_ext_data | hci_dev_unlock() | ... | hci_dev_lock() if (hdev->smp_data) | chan = hdev->smp_data | | chan = hdev->smp_data (3) | hdev->smp_data = NULL (1) | if (!chan || !chan->data) (4) ... | smp = chan->data | smp = chan->data if (smp) | chan->data = NULL (2) | ... | kfree_sensitive(smp) | | // dereference smp trigger UFA That is, the objects hdev->smp_data and chan->data both suffer from the data races. In a preempt-enable kernel, the above schedule (when (3) is before (1) and (4) is before (2)) leads to UAF bugs. It can be reproduced in the latest kernel and below is part of the report: [ 49.097146] ================================================================ [ 49.097611] BUG: KASAN: use-after-free in smp_generate_oob+0x2dd/0x570 [ 49.097611] Read of size 8 at addr ffff888006528360 by task generate_oob/155 [ 49.097611] [ 49.097611] Call Trace: [ 49.097611] <TASK> [ 49.097611] dump_stack_lvl+0x34/0x44 [ 49.097611] print_address_description.constprop.0+0x1f/0x150 [ 49.097611] ? smp_generate_oob+0x2dd/0x570 [ 49.097611] ? smp_generate_oob+0x2dd/0x570 [ 49.097611] kasan_report.cold+0x7f/0x11b [ 49.097611] ? smp_generate_oob+0x2dd/0x570 [ 49.097611] smp_generate_oob+0x2dd/0x570 [ 49.097611] read_local_oob_ext_data+0x689/0xc30 [ 49.097611] ? hci_event_packet+0xc80/0xc80 [ 49.097611] ? sysvec_apic_timer_interrupt+0x9b/0xc0 [ 49.097611] ? asm_sysvec_apic_timer_interrupt+0x12/0x20 [ 49.097611] ? mgmt_init_hdev+0x1c/0x240 [ 49.097611] ? mgmt_init_hdev+0x28/0x240 [ 49.097611] hci_sock_sendmsg+0x1880/0x1e70 [ 49.097611] ? create_monitor_event+0x890/0x890 [ 49.097611] ? create_monitor_event+0x890/0x890 [ 49.097611] sock_sendmsg+0xdf/0x110 [ 49.097611] __sys_sendto+0x19e/0x270 [ 49.097611] ? __ia32_sys_getpeername+0xa0/0xa0 [ 49.097611] ? kernel_fpu_begin_mask+0x1c0/0x1c0 [ 49.097611] __x64_sys_sendto+0xd8/0x1b0 [ 49.097611] ? syscall_exit_to_user_mode+0x1d/0x40 [ 49.097611] do_syscall_64+0x3b/0x90 [ 49.097611] entry_SYSCALL_64_after_hwframe+0x44/0xae [ 49.097611] RIP: 0033:0x7f5a59f51f64 ... [ 49.097611] RAX: ffffffffffffffda RBX: 0000000000000000 RCX: 00007f5a59f51f64 [ 49.097611] RDX: 0000000000000007 RSI: 00007f5a59d6ac70 RDI: 0000000000000006 [ 49.097611] RBP: 0000000000000000 R08: 0000000000000000 R09: 0000000000000000 [ 49.097611] R10: 0000000000000040 R11: 0000000000000246 R12: 00007ffec26916ee [ 49.097611] R13: 00007ffec26916ef R14: 00007f5a59d6afc0 R15: 00007f5a59d6b700 To solve these data races, this patch places the smp_unregister() function in the protected area by the hci_dev_lock(). That is, the smp_unregister() function can not be concurrently executed when operating functions (most of them are mgmt operations in mgmt.c) hold the device lock. This patch is tested with kernel LOCK DEBUGGING enabled. The price from the extended holding time of the device lock is supposed to be low as the smp_unregister() function is fairly short and efficient. Signed-off-by: Lin Ma <linma@zju.edu.cn> --- net/bluetooth/hci_sync.c | 4 ++-- 1 file changed, 2 insertions(+), 2 deletions(-)