• Lin Ma's avatar
    Bluetooth: fix data races in smp_unregister(), smp_del_chan() · fa78d2d1
    Lin Ma authored
    Previous commit e0448092 ("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: default avatarLin Ma <linma@zju.edu.cn>
    Signed-off-by: default avatarMarcel Holtmann <marcel@holtmann.org>
    fa78d2d1
hci_sync.c 138 KB