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
