Re: Re: [PATCH v1] Bluetooth: fix data races in smp_unregister(), smp_del_chan()

[Date Prev][Date Next][Thread Prev][Thread Next][Date Index][Thread Index]

 



Hi Marcel,

Thanks for the timely reply.

Regards

Lin


> -----Original Messages-----
> From: "Marcel Holtmann" <marcel@xxxxxxxxxxxx>
> Sent Time: 2022-02-16 18:23:52 (Wednesday)
> To: "Lin Ma" <linma@xxxxxxxxxx>
> Cc: "Johan Hedberg" <johan.hedberg@xxxxxxxxx>, "Luiz Augusto von Dentz" <luiz.dentz@xxxxxxxxx>, "David S. Miller" <davem@xxxxxxxxxxxxx>, linux-bluetooth@xxxxxxxxxxxxxxx
> 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@xxxxxxxxxx>
> > ---
> > net/bluetooth/hci_sync.c | 4 ++--
> > 1 file changed, 2 insertions(+), 2 deletions(-)
> 
> patch has been applied to bluetooth-next tree.
> 
> Regards
> 
> Marcel




[Index of Archives]     [Bluez Devel]     [Linux Wireless Networking]     [Linux Wireless Personal Area Networking]     [Linux ATH6KL]     [Linux USB Devel]     [Linux Media Drivers]     [Linux Audio Users]     [Linux Kernel]     [Linux SCSI]     [Big List of Linux Books]

  Powered by Linux