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 <[email protected]>
---
net/bluetooth/hci_sync.c | 4 ++--
1 file changed, 2 insertions(+), 2 deletions(-)
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);
--
2.33.1
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 <[email protected]>
> ---
> 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" <[email protected]>
> Sent Time: 2022-02-16 18:23:52 (Wednesday)
> To: "Lin Ma" <[email protected]>
> Cc: "Johan Hedberg" <[email protected]>, "Luiz Augusto von Dentz" <[email protected]>, "David S. Miller" <[email protected]>, [email protected]
> 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 <[email protected]>
> > ---
> > net/bluetooth/hci_sync.c | 4 ++--
> > 1 file changed, 2 insertions(+), 2 deletions(-)
>
> patch has been applied to bluetooth-next tree.
>
> Regards
>
> Marcel