[lvc-project] [PATCH v2] Bluetooth: fix use-after-free in device_for_each_child()

[Dmitry Antipov]

On 11/1/24 8:37 PM, Luiz Augusto von Dentz wrote:

> Looks like this doesn't solve the problem, in fact I think you are
> getting it backwards, you are trying to reparent the parent dev not
> the child and I assume by destroying the parent device there should be
> some way to reset the parent which seems to be the intent the
> following code in hci_conn_del_sysfs:
> 
>      while (1) {
>          struct device *dev;
> 
>          dev = device_find_child(&conn->dev, NULL, __match_tty);
>          if (!dev)
>              break;
>          device_move(dev, NULL, DPM_ORDER_DEV_LAST);
>          put_device(dev);
>      }
> 
> But note that it only does that after matching tty, but I guess we
> want to do it regardless otherwise we may have the child objects still
> access it, that said we should probably use device_for_each_child
> though if that is safe to do calls to device_move under its callback.

I'm not sure that I've got your point. IIUC the problem is that a controller
(parent) instance may be freed _after_ the child (connection) has passed
'device_unregister(&conn->dev)' but _before_ an actual removal of 'conn->dev'
from the devices hierarchy, thus leaving the dangling 'conn->dev.parent'
pointer. The latter may be fixed by reparenting 'conn->dev' to NULL explicitly.
And handling children of 'conn->dev' (i.e. the grandchilren of the controller)
is out of this problem's scope at all.

And nothing to say about syzbot's innards but manual testing shows that the
following thing:

void hci_conn_del_sysfs(struct hci_conn *conn)
{
         struct hci_dev *hdev = conn->hdev;

         bt_dev_dbg(hdev, "conn %p", conn);

         if (!device_is_registered(&conn->dev)) {
                 /* If device_add() has *not* succeeded, use *only* put_device()
                  * to drop the reference count.
                  */
                 put_device(&conn->dev);
                 return;
         }

         while (1) {
                 struct device *dev;

                 dev = device_find_any_child(&conn->dev);
                 if (!dev)
                         break;
                 printk(KERN_ERR "%s:%d: reparent dev@%p(%s) with parent@%p(%s)\n",
                        __FILE__, __LINE__, dev, dev_name(dev), dev->parent,
                        (dev->parent ? dev_name(dev->parent) : "<none>"));
                 device_move(dev, NULL, DPM_ORDER_DEV_LAST);
                 put_device(dev);
         }

         device_unregister(&conn->dev);
}

occasionally triggers the following crash:

net/bluetooth/hci_sysfs.c:82: reparent dev at ffff888114be86f8(bnep0) with parent at ffff888111c64b68(hci4:200)
Oops: general protection fault, probably for non-canonical address 0xdffffc000000000b: 0000 [#1] PREEMPT SMP KASI
KASAN: null-ptr-deref in range [0x0000000000000058-0x000000000000005f]
CPU: 3 UID: 0 PID: 6033 Comm: repro Not tainted 6.12.0-rc5-00299-g11066801dd4b-dirty #8
Hardware name: QEMU Standard PC (i440FX + PIIX, 1996), BIOS 1.16.3-3.fc41 04/01/2014
RIP: 0010:klist_put+0x4d/0x1d0
Code: c1 ea 03 80 3c 02 00 0f 85 74 01 00 00 48 b8 00 00 00 00 00 fc ff df 4c 8b 23 49 83 e4 fe 49 8d 7c 24 58 49
RSP: 0018:ffffc9000423f9b0 EFLAGS: 00010202
RAX: dffffc0000000000 RBX: ffff88810f43ac60 RCX: 0000000000000000
RDX: 000000000000000b RSI: ffffffff8a92d415 RDI: 0000000000000058
RBP: 0000000000000001 R08: 0000000000000000 R09: fffffbfff1fad62c
R10: ffffffff8fd6b163 R11: 0000000000000000 R12: 0000000000000000
R13: 0000000000000001 R14: ffffc9000423fa30 R15: ffffffff8a92d805
FS:  00007f24ebb78740(0000) GS:ffff888135f00000(0000) knlGS:0000000000000000
CS:  0010 DS: 0000 ES: 0000 CR0: 0000000080050033
CR2: 000055c52afb6950 CR3: 0000000020c1c000 CR4: 00000000000006f0
Call Trace:
  <TASK>
  ? die_addr.cold+0x8/0xd
  ? exc_general_protection+0x147/0x240
  ? asm_exc_general_protection+0x26/0x30
  ? klist_remove+0x155/0x2b0
  ? klist_put+0x15/0x1d0
  ? klist_put+0x4d/0x1d0
  klist_remove+0x15a/0x2b0
  ? __pfx_klist_remove+0x10/0x10
  device_move+0x12d/0x10b0
  hci_conn_del_sysfs.cold+0xcf/0x14a
  hci_conn_del+0x467/0xd60
  hci_conn_hash_flush+0x18f/0x270
  hci_dev_close_sync+0x549/0x1260
  hci_dev_do_close+0x2e/0x90
  hci_unregister_dev+0x213/0x630
  vhci_release+0x79/0xf0
  ? __pfx_vhci_release+0x10/0x10
  __fput+0x3f6/0xb30
  task_work_run+0x151/0x250
  ? __pfx_task_work_run+0x10/0x10
  do_exit+0xa79/0x2c30
  ? do_raw_spin_lock+0x12a/0x2b0
  ? __pfx_do_exit+0x10/0x10
  do_group_exit+0xd5/0x2a0
  __x64_sys_exit_group+0x3e/0x50
  x64_sys_call+0x14af/0x14b0
  do_syscall_64+0xc7/0x250
  entry_SYSCALL_64_after_hwframe+0x77/0x7f

Dmitry