[PATCH v3] Bluetooth: vhci: Fix slab-use-after-free by cloning skb

Szymon Wilczek posted 1 patch 1 month, 1 week ago
drivers/bluetooth/hci_vhci.c | 11 +++++++++--
1 file changed, 9 insertions(+), 2 deletions(-)
[PATCH v3] Bluetooth: vhci: Fix slab-use-after-free by cloning skb
Posted by Szymon Wilczek 1 month, 1 week ago
Hillf Danton pointed out that the root cause of the UAF issue is the
lack of isolation between hci_core and vhci driver consumers.

vhci_send_frame() modifies the skb (via skb_push) and queues the
original skb to the readq for userspace consumption. This means the
hci_core caller sees a modified skb (corrupted headers/data pointer)
if it retains any reference. Furthermore, if vhci_read() frees the
skb immediately, hci_core might hit a Use-After-Free.

Other drivers (like btusb) create a new URB and context, isolating
the skb lifetime.

Fix this by cloning the skb in vhci_send_frame() before queueing.
The clone is modified and queued. The original skb is freed using
dev_consume_skb_any() which is safe in atomic context, satisfying
the HCI driver contract to consume the skb while ensuring the queued
object is distinct from the one passed by hci_core.

Reported-by: syzbot+4d6b203d625d2f57d4ca@syzkaller.appspotmail.com
Closes: https://syzkaller.appspot.com/bug?extid=4d6b203d625d2f57d4ca
Signed-off-by: Szymon Wilczek <swilczek.lx@gmail.com>
---
v3: Replaced kfree_skb() with dev_consume_skb_any() to fix sleeping
    in atomic context warning reported by CI.
v2: Moved fix to vhci driver, using skb_clone to isolate ownership.
---
 drivers/bluetooth/hci_vhci.c | 11 +++++++++--
 1 file changed, 9 insertions(+), 2 deletions(-)

diff --git a/drivers/bluetooth/hci_vhci.c b/drivers/bluetooth/hci_vhci.c
index 2fef08254d78..7c72c635965c 100644
--- a/drivers/bluetooth/hci_vhci.c
+++ b/drivers/bluetooth/hci_vhci.c
@@ -74,13 +74,20 @@ static int vhci_flush(struct hci_dev *hdev)
 static int vhci_send_frame(struct hci_dev *hdev, struct sk_buff *skb)
 {
 	struct vhci_data *data = hci_get_drvdata(hdev);
+	struct sk_buff *nskb;
 
-	memcpy(skb_push(skb, 1), &hci_skb_pkt_type(skb), 1);
+	nskb = skb_clone(skb, GFP_ATOMIC);
+	if (!nskb)
+		return -ENOMEM;
+
+	memcpy(skb_push(nskb, 1), &hci_skb_pkt_type(skb), 1);
 
-	skb_queue_tail(&data->readq, skb);
+	skb_queue_tail(&data->readq, nskb);
 
 	if (atomic_read(&data->initialized))
 		wake_up_interruptible(&data->read_wait);
+
+	dev_consume_skb_any(skb);
 	return 0;
 }
 
-- 
2.52.0