| Message ID | 20241007074538.109613-1-dmantipov@yandex.ru |
|---|---|
| State | New |
| Headers | show |
| Series | Bluetooth: do not send mgmt commands to device which is going to close | expand |
Hi Dmitry, On Mon, 07. Oct 10:45, Dmitry Antipov wrote: > Syzbot has observed the following race between 'hci_dev_close()' and > 'hci_cmd_sync_work()': > > T0: T1: > > ... > -> sock_ioctl() > -> sock_do_ioctl() > -> hci_dev_close() > -> hci_dev_close_sync() > -> __mgmt_power_off() ... > -> mgmt_pending_foreach() -> process_scheduled_works() > -> settings_rsp() -> hci_cmd_sync_work() > -> kfree() -> set_powered_sync() I guess commit f53e1c9c726d ("Bluetooth: MGMT: Fix possible crash on mgmt_index_removed") [1] is supposed to fix the observed race. Is there something missing? [1]: https://git.kernel.org/torvalds/c/f53e1c9c726d83092167f2226f32bd3b73f26c21 > Reported-by: syzbot+03d6270b6425df1605bf@syzkaller.appspotmail.com > Closes: https://syzkaller.appspot.com/bug?extid=03d6270b6425df1605bf Btw, `Fixes` tag is really desirable if you are fixing bugs in kernel, like KASAN splats and others. > Signed-off-by: Dmitry Antipov <dmantipov@yandex.ru> > ---
diff --git a/include/net/bluetooth/hci.h b/include/net/bluetooth/hci.h index bab1e3d7452a..492723a22e68 100644 --- a/include/net/bluetooth/hci.h +++ b/include/net/bluetooth/hci.h @@ -345,6 +345,7 @@ enum { HCI_UP, HCI_INIT, HCI_RUNNING, + HCI_CLOSING, HCI_PSCAN, HCI_ISCAN, diff --git a/net/bluetooth/hci_core.c b/net/bluetooth/hci_core.c index 629c302f7407..95f55cfb6da6 100644 --- a/net/bluetooth/hci_core.c +++ b/net/bluetooth/hci_core.c @@ -501,12 +501,16 @@ int hci_dev_close(__u16 dev) goto done; } + set_bit(HCI_CLOSING, &hdev->flags); + cancel_work_sync(&hdev->power_on); if (hci_dev_test_and_clear_flag(hdev, HCI_AUTO_OFF)) cancel_delayed_work(&hdev->power_off); err = hci_dev_do_close(hdev); + if (unlikely(err)) + clear_bit(HCI_CLOSING, &hdev->flags); done: hci_dev_put(hdev); return err; diff --git a/net/bluetooth/hci_sock.c b/net/bluetooth/hci_sock.c index 2272e1849ebd..ff43718822d4 100644 --- a/net/bluetooth/hci_sock.c +++ b/net/bluetooth/hci_sock.c @@ -1671,6 +1671,11 @@ static int hci_mgmt_cmd(struct hci_mgmt_chan *chan, struct sock *sk, goto done; } + if (unlikely(test_bit(HCI_CLOSING, &hdev->flags))) { + err = -ENODEV; + goto done; + } + if (hci_dev_test_flag(hdev, HCI_SETUP) || hci_dev_test_flag(hdev, HCI_CONFIG) || hci_dev_test_flag(hdev, HCI_USER_CHANNEL)) {
Syzbot has observed the following race between 'hci_dev_close()' and 'hci_cmd_sync_work()': T0: T1: ... -> sock_ioctl() -> sock_do_ioctl() -> hci_dev_close() -> hci_dev_close_sync() -> __mgmt_power_off() ... -> mgmt_pending_foreach() -> process_scheduled_works() -> settings_rsp() -> hci_cmd_sync_work() -> kfree() -> set_powered_sync() That is, 'hci_cmd_sync_work()' makes an attempt to process a command from (partially) freed 'cmd_sync_work_list', which causes UAF detected by KASAN. Fix this by marking the closing device with HCI_CLOSING bit very early and rejecting new mgmt commands for such a device. Reported-by: syzbot+03d6270b6425df1605bf@syzkaller.appspotmail.com Closes: https://syzkaller.appspot.com/bug?extid=03d6270b6425df1605bf Signed-off-by: Dmitry Antipov <dmantipov@yandex.ru> --- include/net/bluetooth/hci.h | 1 + net/bluetooth/hci_core.c | 4 ++++ net/bluetooth/hci_sock.c | 5 +++++ 3 files changed, 10 insertions(+)