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

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

[Lin Ma]

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@zju.edu.cn>
---
 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

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

[bluez.test.bot]

[-- Attachment #1: Type: text/plain, Size: 1377 bytes --]

This is automated email and please do not reply to this email!

Dear submitter,

Thank you for submitting the patches to the linux bluetooth mailing list.
This is a CI test results with your patch series:
PW Link:https://patchwork.kernel.org/project/bluetooth/list/?series=614795

---Test result---

Test Summary:
CheckPatch                    PASS      0.77 seconds
GitLint                       PASS      0.42 seconds
SubjectPrefix                 PASS      0.24 seconds
BuildKernel                   PASS      36.72 seconds
BuildKernel32                 PASS      31.95 seconds
Incremental Build with patchesPASS      43.85 seconds
TestRunner: Setup             PASS      576.01 seconds
TestRunner: l2cap-tester      PASS      15.71 seconds
TestRunner: bnep-tester       PASS      7.06 seconds
TestRunner: mgmt-tester       PASS      122.73 seconds
TestRunner: rfcomm-tester     FAIL      8.94 seconds
TestRunner: sco-tester        PASS      9.15 seconds
TestRunner: smp-tester        PASS      9.07 seconds
TestRunner: userchan-tester   PASS      7.41 seconds

Details
##############################
Test: TestRunner: rfcomm-tester - FAIL - 8.94 seconds
Run test-runner with rfcomm-tester
Total: 10, Passed: 9 (90.0%), Failed: 1, Not Run: 0

Failed Test Cases
Basic RFCOMM Socket Client - Write 32k Success       Failed       0.192 seconds



---
Regards,
Linux Bluetooth

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

[Marcel Holtmann]

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@zju.edu.cn>
> ---
> net/bluetooth/hci_sync.c | 4 ++--
> 1 file changed, 2 insertions(+), 2 deletions(-)

patch has been applied to bluetooth-next tree.

Regards

Marcel

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

[Lin Ma]

Hi Marcel,

Thanks for the timely reply.

Regards

Lin


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