| lists.openwall.net | lists / announce owl-users owl-dev john-users john-dev passwdqc-users yescrypt popa3d-users / oss-security kernel-hardening musl sabotage tlsify passwords / crypt-dev xvendor / Bugtraq Full-Disclosure linux-kernel linux-netdev linux-ext4 linux-hardening linux-cve-announce PHC | |
|
Open Source and information security mailing list archives
| ||
|
Message-ID: <20241205071727.36710-2-en-wei.wu@canonical.com>
Date: Thu, 5 Dec 2024 15:17:26 +0800
From: En-Wei Wu <en-wei.wu@...onical.com>
To: marcel@...tmann.org,
luiz.dentz@...il.com,
linux-bluetooth@...r.kernel.org,
linux-kernel@...r.kernel.org,
pmenzel@...gen.mpg.de
Cc: quic_tjiang@...cinc.com,
kuan-ying.lee@...onical.com,
anthony.wong@...onical.com
Subject: [PATCH v3 1/2] Bluetooth: btusb: avoid NULL pointer dereference in skb_dequeue()
The WCN7851 (0489:e0f3) Bluetooth controller supports firmware crash dump
collection through devcoredump. During this process, the crash dump data
is queued to a dump queue as skb for further processing.
A NULL pointer dereference occurs in skb_dequeue() when processing the
dump queue due to improper return value handling:
[ 93.672166] Bluetooth: hci0: ACL memdump size(589824)
[ 93.672475] BUG: kernel NULL pointer dereference, address: 0000000000000008
[ 93.672517] Workqueue: hci0 hci_devcd_rx [bluetooth]
[ 93.672598] RIP: 0010:skb_dequeue+0x50/0x80
The issue stems from handle_dump_pkt_qca() returning the wrong value on
success. It currently returns the value from hci_devcd_init()
(0 on success), but callers expect > 0 to indicate successful
dump handling. This causes hci_recv_frame() to free the skb while
it's still queued for dump processing, leading to the NULL pointer
dereference when hci_devcd_rx() tries to dequeue it.
Fix this by:
1. Making handle_dump_pkt_qca() return 0 on success and negative errno
on failure, consistent with other kernel interfaces
2. Splitting dump packet detection into separate ACL and event functions
for better code organization and maintainability
This prevents premature skb freeing by ensuring proper handling
of dump packets.
Fixes: 20981ce2d5a5 ("Bluetooth: btusb: Add WCN6855 devcoredump support")
Signed-off-by: En-Wei Wu <en-wei.wu@...onical.com>
---
drivers/bluetooth/btusb.c | 97 ++++++++++++++++++++++++++++-----------
1 file changed, 69 insertions(+), 28 deletions(-)
diff --git a/drivers/bluetooth/btusb.c b/drivers/bluetooth/btusb.c
index 279fe6c115fa..2bfb915062cf 100644
--- a/drivers/bluetooth/btusb.c
+++ b/drivers/bluetooth/btusb.c
@@ -2930,22 +2930,16 @@ static void btusb_coredump_qca(struct hci_dev *hdev)
bt_dev_err(hdev, "%s: triggle crash failed (%d)", __func__, err);
}
-/*
- * ==0: not a dump pkt.
- * < 0: fails to handle a dump pkt
- * > 0: otherwise.
- */
+/* Return: 0 on success, negative errno on failure. */
static int handle_dump_pkt_qca(struct hci_dev *hdev, struct sk_buff *skb)
{
- int ret = 1;
+ int ret = 0;
u8 pkt_type;
u8 *sk_ptr;
unsigned int sk_len;
u16 seqno;
u32 dump_size;
- struct hci_event_hdr *event_hdr;
- struct hci_acl_hdr *acl_hdr;
struct qca_dump_hdr *dump_hdr;
struct btusb_data *btdata = hci_get_drvdata(hdev);
struct usb_device *udev = btdata->udev;
@@ -2955,30 +2949,14 @@ static int handle_dump_pkt_qca(struct hci_dev *hdev, struct sk_buff *skb)
sk_len = skb->len;
if (pkt_type == HCI_ACLDATA_PKT) {
- acl_hdr = hci_acl_hdr(skb);
- if (le16_to_cpu(acl_hdr->handle) != QCA_MEMDUMP_ACL_HANDLE)
- return 0;
sk_ptr += HCI_ACL_HDR_SIZE;
sk_len -= HCI_ACL_HDR_SIZE;
- event_hdr = (struct hci_event_hdr *)sk_ptr;
- } else {
- event_hdr = hci_event_hdr(skb);
}
- if ((event_hdr->evt != HCI_VENDOR_PKT)
- || (event_hdr->plen != (sk_len - HCI_EVENT_HDR_SIZE)))
- return 0;
-
sk_ptr += HCI_EVENT_HDR_SIZE;
sk_len -= HCI_EVENT_HDR_SIZE;
dump_hdr = (struct qca_dump_hdr *)sk_ptr;
- if ((sk_len < offsetof(struct qca_dump_hdr, data))
- || (dump_hdr->vse_class != QCA_MEMDUMP_VSE_CLASS)
- || (dump_hdr->msg_type != QCA_MEMDUMP_MSG_TYPE))
- return 0;
-
- /*it is dump pkt now*/
seqno = le16_to_cpu(dump_hdr->seqno);
if (seqno == 0) {
set_bit(BTUSB_HW_SSR_ACTIVE, &btdata->flags);
@@ -3052,17 +3030,80 @@ static int handle_dump_pkt_qca(struct hci_dev *hdev, struct sk_buff *skb)
return ret;
}
+/* Return: true if the ACL packet is a dump packet, false otherwise. */
+static bool btqca_acl_pkt_is_dump(struct hci_dev *hdev, struct sk_buff *skb)
+{
+ struct hci_event_hdr *event_hdr;
+ struct hci_acl_hdr *acl_hdr;
+ struct qca_dump_hdr *dump_hdr;
+
+ sk_ptr = skb->data;
+ sk_len = skb->len;
+
+ acl_hdr = hci_acl_hdr(skb);
+ if (le16_to_cpu(acl_hdr->handle) != QCA_MEMDUMP_ACL_HANDLE)
+ return false;
+ sk_ptr += HCI_ACL_HDR_SIZE;
+ sk_len -= HCI_ACL_HDR_SIZE;
+ event_hdr = (struct hci_event_hdr *)sk_ptr;
+
+ if ((event_hdr->evt != HCI_VENDOR_PKT)
+ || (event_hdr->plen != (sk_len - HCI_EVENT_HDR_SIZE)))
+ return false;
+
+ sk_ptr += HCI_EVENT_HDR_SIZE;
+ sk_len -= HCI_EVENT_HDR_SIZE;
+
+ dump_hdr = (struct qca_dump_hdr *)sk_ptr;
+ if ((sk_len < offsetof(struct qca_dump_hdr, data))
+ || (dump_hdr->vse_class != QCA_MEMDUMP_VSE_CLASS)
+ || (dump_hdr->msg_type != QCA_MEMDUMP_MSG_TYPE))
+ return false;
+
+ return true;
+}
+
+/* Return: true if the event packet is a dump packet, false otherwise. */
+static bool btqca_event_pkt_is_dump(struct hci_dev *hdev, struct sk_buff *skb)
+{
+ u8 *sk_ptr;
+ unsigned int sk_len;
+
+ struct hci_event_hdr *event_hdr;
+ struct qca_dump_hdr *dump_hdr;
+
+ sk_ptr = skb->data;
+ sk_len = skb->len;
+
+ event_hdr = hci_event_hdr(skb);
+
+ if ((event_hdr->evt != HCI_VENDOR_PKT)
+ || (event_hdr->plen != (sk_len - HCI_EVENT_HDR_SIZE)))
+ return false;
+
+ sk_ptr += HCI_EVENT_HDR_SIZE;
+ sk_len -= HCI_EVENT_HDR_SIZE;
+
+ dump_hdr = (struct qca_dump_hdr *)sk_ptr;
+ if ((sk_len < offsetof(struct qca_dump_hdr, data))
+ || (dump_hdr->vse_class != QCA_MEMDUMP_VSE_CLASS)
+ || (dump_hdr->msg_type != QCA_MEMDUMP_MSG_TYPE))
+ return false;
+
+ return true;
+}
+
static int btusb_recv_acl_qca(struct hci_dev *hdev, struct sk_buff *skb)
{
- if (handle_dump_pkt_qca(hdev, skb))
- return 0;
+ if (btqca_acl_pkt_is_dump(hdev, skb))
+ return handle_dump_pkt_qca(hdev, skb);
return hci_recv_frame(hdev, skb);
}
static int btusb_recv_evt_qca(struct hci_dev *hdev, struct sk_buff *skb)
{
- if (handle_dump_pkt_qca(hdev, skb))
- return 0;
+ if (btqca_event_pkt_is_dump(hdev, skb))
+ return handle_dump_pkt_qca(hdev, skb);
return hci_recv_frame(hdev, skb);
}
--
2.43.0
Powered by blists - more mailing lists