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@xxxxxxxxxx>
> ---
> net/bluetooth/hci_sync.c | 4 ++--
> 1 file changed, 2 insertions(+), 2 deletions(-)
patch has been applied to bluetooth-next tree.
Regards
Marcel
