[PATCH] Bluetooth: hci_event: Synchronously cancel timers in hci_cmd_complete_evt()

[Sungwoo Kim]

RFC only.

hci_cmd_complete_evt() and hci_cmd_timeout can interleave, leading to
user-after-free access.

CPU1                  CPU2
hci_cmd_timeout()
                      hci_event_packet()
                      [snip]
                        hci_cmd_complete_evt()
                          handle_cmd_cnt_and_timer()
                            // this is asynchronous
                            cancel_delayed_work(&hdev->cmd_timer)
                        hci_cmd_sync_complete()
                          kfree_skb(hdev->req_skb); // free
  hci_skb_opcode(hdev->req_skb); // use-after-free

To fix this, make cancel_delayed_work() synchronous so it can wait for
the timeout handler.
However, this is not a complete fix because hci_cmd_timeout() resets the
device and queue a new command.
I would like to request for comments the better way to fix this issue.

KASAN splat:
BUG: KASAN: slab-use-after-free in hci_cmd_timeout+0x216/0x260 net/bluetooth/hci_core.c:1432
Read of size 2 at addr ffff88811605a7b8 by task syz.2.21760/74233

Call Trace:
 <TASK>
 __dump_stack lib/dump_stack.c:94 [inline]
 dump_stack_lvl+0xba/0x110 lib/dump_stack.c:120
 print_address_description mm/kasan/report.c:378 [inline]
 print_report+0x157/0x4c9 mm/kasan/report.c:482
 kasan_report+0xdf/0x1b0 mm/kasan/report.c:595
 hci_cmd_timeout+0x216/0x260 net/bluetooth/hci_core.c:1432
[snip]

Freed by task 4563:
[snip]
 kfree_skb include/linux/skbuff.h:1333 [inline]
 hci_cmd_sync_complete net/bluetooth/hci_sync.c:36 [inline]
 hci_cmd_sync_complete+0x152/0x370 net/bluetooth/hci_sync.c:24
 hci_event_packet+0x8fd/0xd20 net/bluetooth/hci_event.c:7863
 hci_rx_work+0x5c5/0xfa0 net/bluetooth/hci_core.c:4041
 process_one_work+0x93f/0x1810 kernel/workqueue.c:3316
[snip]

Fixes: ecb71f256667 ("Bluetooth: Fix race condition in handling NOP command")
Acked-by: Dave Tian <daveti@xxxxxxxxxx>
Signed-off-by: Sungwoo Kim <iam@xxxxxxxxxxxx>
---
 net/bluetooth/hci_event.c | 10 +++++-----
 1 file changed, 5 insertions(+), 5 deletions(-)

diff --git a/net/bluetooth/hci_event.c b/net/bluetooth/hci_event.c
index eea2f810aafa..3639aa896bc3 100644
--- a/net/bluetooth/hci_event.c
+++ b/net/bluetooth/hci_event.c
@@ -3765,14 +3765,14 @@ static void hci_remote_features_evt(struct hci_dev *hdev, void *data,
 	hci_dev_unlock(hdev);
 }
 
-static inline void handle_cmd_cnt_and_timer(struct hci_dev *hdev, u8 ncmd)
+static inline void handle_cmd_cnt_and_timer_sync(struct hci_dev *hdev, u8 ncmd)
 {
-	cancel_delayed_work(&hdev->cmd_timer);
+	cancel_delayed_work_sync(&hdev->cmd_timer);
 
 	rcu_read_lock();
 	if (!test_bit(HCI_RESET, &hdev->flags)) {
 		if (ncmd) {
-			cancel_delayed_work(&hdev->ncmd_timer);
+			cancel_delayed_work_sync(&hdev->ncmd_timer);
 			atomic_set(&hdev->cmd_cnt, 1);
 		} else {
 			if (!hci_dev_test_flag(hdev, HCI_CMD_DRAIN_WORKQUEUE))
@@ -4304,7 +4304,7 @@ static void hci_cmd_complete_evt(struct hci_dev *hdev, void *data,
 		*status = skb->data[0];
 	}
 
-	handle_cmd_cnt_and_timer(hdev, ev->ncmd);
+	handle_cmd_cnt_and_timer_sync(hdev, ev->ncmd);
 
 	hci_req_cmd_complete(hdev, *opcode, *status, req_complete,
 			     req_complete_skb);
@@ -4418,7 +4418,7 @@ static void hci_cmd_status_evt(struct hci_dev *hdev, void *data,
 		}
 	}
 
-	handle_cmd_cnt_and_timer(hdev, ev->ncmd);
+	handle_cmd_cnt_and_timer_sync(hdev, ev->ncmd);
 
 	/* Indicate request completion if the command failed. Also, if
 	 * we're not waiting for a special event and we get a success
-- 
2.47.3