* [PATCH v4 1/2] Bluetooth: btusb: avoid NULL pointer dereference in skb_dequeue()
2025-04-21 13:00 [PATCH v4 0/2] btusb: fix NULL pointer dereference in QCA devcoredump handling En-Wei Wu
@ 2025-04-21 13:00 ` En-Wei Wu
2025-04-21 14:01 ` btusb: fix NULL pointer dereference in QCA devcoredump handling bluez.test.bot
2025-04-21 13:00 ` [PATCH v4 2/2] Bluetooth: btusb: use skb_pull to avoid unsafe access in QCA dump handling En-Wei Wu
2025-04-21 19:17 ` [PATCH v4 0/2] btusb: fix NULL pointer dereference in QCA devcoredump handling Luiz Augusto von Dentz
2 siblings, 1 reply; 8+ messages in thread
From: En-Wei Wu @ 2025-04-21 13:00 UTC (permalink / raw)
To: marcel, luiz.dentz, linux-bluetooth, linux-kernel, pmenzel
Cc: quic_tjiang, chia-lin.kao, anthony.wong
A NULL pointer dereference can occur in skb_dequeue() when processing a
QCA firmware crash dump on WCN7851 (0489:e0f3).
[ 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 0 even when a dump
packet is successfully processed. This is because it incorrectly
forwards the return value of hci_devcd_init() (which returns 0 on
success). As a result, the caller (btusb_recv_acl_qca() or
btusb_recv_evt_qca()) assumes the packet was not handled and passes it
to hci_recv_frame(), leading to premature kfree() of the skb.
Later, hci_devcd_rx() attempts to dequeue the same skb from the dump
queue, resulting in a NULL pointer dereference.
Fix this by:
1. Making handle_dump_pkt_qca() return 0 on success and negative errno
on failure, consistent with kernel conventions.
2. Splitting dump packet detection into separate functions for ACL
and event packets for better structure and readability.
This ensures dump packets are properly identified and consumed, avoiding
double handling and preventing NULL pointer access.
Fixes: 20981ce2d5a5 ("Bluetooth: btusb: Add WCN6855 devcoredump support")
Signed-off-by: En-Wei Wu <en-wei.wu@canonical.com>
---
drivers/bluetooth/btusb.c | 100 +++++++++++++++++++++++++++-----------
1 file changed, 72 insertions(+), 28 deletions(-)
diff --git a/drivers/bluetooth/btusb.c b/drivers/bluetooth/btusb.c
index 7a9b89bcea22..f905780fcdea 100644
--- a/drivers/bluetooth/btusb.c
+++ b/drivers/bluetooth/btusb.c
@@ -3012,22 +3012,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;
@@ -3037,30 +3031,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);
@@ -3134,17 +3112,83 @@ 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 acl_pkt_is_dump_qca(struct hci_dev *hdev, struct sk_buff *skb)
+{
+ u8 *sk_ptr;
+ unsigned int sk_len;
+
+ 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 evt_pkt_is_dump_qca(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 (acl_pkt_is_dump_qca(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 (evt_pkt_is_dump_qca(hdev, skb))
+ return handle_dump_pkt_qca(hdev, skb);
return hci_recv_frame(hdev, skb);
}
--
2.43.0
^ permalink raw reply related [flat|nested] 8+ messages in thread* RE: btusb: fix NULL pointer dereference in QCA devcoredump handling
2025-04-21 13:00 ` [PATCH v4 1/2] Bluetooth: btusb: avoid NULL pointer dereference in skb_dequeue() En-Wei Wu
@ 2025-04-21 14:01 ` bluez.test.bot
0 siblings, 0 replies; 8+ messages in thread
From: bluez.test.bot @ 2025-04-21 14:01 UTC (permalink / raw)
To: linux-bluetooth, en-wei.wu
[-- Attachment #1: Type: text/plain, Size: 1681 bytes --]
This is automated email and please do not reply to this email!
Dear submitter,
Thank you for submitting the patches to the linux bluetooth mailing list.
This is a CI test results with your patch series:
PW Link:https://patchwork.kernel.org/project/bluetooth/list/?series=955286
---Test result---
Test Summary:
CheckPatch PENDING 0.33 seconds
GitLint PENDING 0.25 seconds
SubjectPrefix PASS 0.17 seconds
BuildKernel PASS 24.43 seconds
CheckAllWarning PASS 26.77 seconds
CheckSparse PASS 30.50 seconds
BuildKernel32 PASS 24.04 seconds
TestRunnerSetup PASS 457.88 seconds
TestRunner_l2cap-tester PASS 21.04 seconds
TestRunner_iso-tester PASS 35.13 seconds
TestRunner_bnep-tester PASS 4.75 seconds
TestRunner_mgmt-tester PASS 122.35 seconds
TestRunner_rfcomm-tester PASS 7.82 seconds
TestRunner_sco-tester PASS 12.60 seconds
TestRunner_ioctl-tester PASS 8.36 seconds
TestRunner_mesh-tester PASS 6.08 seconds
TestRunner_smp-tester PASS 7.18 seconds
TestRunner_userchan-tester PASS 5.04 seconds
IncrementalBuild PENDING 0.56 seconds
Details
##############################
Test: CheckPatch - PENDING
Desc: Run checkpatch.pl script
Output:
##############################
Test: GitLint - PENDING
Desc: Run gitlint
Output:
##############################
Test: IncrementalBuild - PENDING
Desc: Incremental build with the patches in the series
Output:
---
Regards,
Linux Bluetooth
^ permalink raw reply [flat|nested] 8+ messages in thread
* [PATCH v4 2/2] Bluetooth: btusb: use skb_pull to avoid unsafe access in QCA dump handling
2025-04-21 13:00 [PATCH v4 0/2] btusb: fix NULL pointer dereference in QCA devcoredump handling En-Wei Wu
2025-04-21 13:00 ` [PATCH v4 1/2] Bluetooth: btusb: avoid NULL pointer dereference in skb_dequeue() En-Wei Wu
@ 2025-04-21 13:00 ` En-Wei Wu
2025-04-21 15:29 ` Luiz Augusto von Dentz
2025-04-21 19:17 ` [PATCH v4 0/2] btusb: fix NULL pointer dereference in QCA devcoredump handling Luiz Augusto von Dentz
2 siblings, 1 reply; 8+ messages in thread
From: En-Wei Wu @ 2025-04-21 13:00 UTC (permalink / raw)
To: marcel, luiz.dentz, linux-bluetooth, linux-kernel, pmenzel
Cc: quic_tjiang, chia-lin.kao, anthony.wong
Use skb_pull() and skb_pull_data() to safely parse QCA dump packets.
This avoids direct pointer math on skb->data, which could lead to
invalid access if the packet is shorter than expected.
Signed-off-by: En-Wei Wu <en-wei.wu@canonical.com>
---
drivers/bluetooth/btusb.c | 98 ++++++++++++++++-----------------------
1 file changed, 41 insertions(+), 57 deletions(-)
diff --git a/drivers/bluetooth/btusb.c b/drivers/bluetooth/btusb.c
index f905780fcdea..031292ab766f 100644
--- a/drivers/bluetooth/btusb.c
+++ b/drivers/bluetooth/btusb.c
@@ -3017,8 +3017,6 @@ static int handle_dump_pkt_qca(struct hci_dev *hdev, struct sk_buff *skb)
{
int ret = 0;
u8 pkt_type;
- u8 *sk_ptr;
- unsigned int sk_len;
u16 seqno;
u32 dump_size;
@@ -3027,18 +3025,8 @@ static int handle_dump_pkt_qca(struct hci_dev *hdev, struct sk_buff *skb)
struct usb_device *udev = btdata->udev;
pkt_type = hci_skb_pkt_type(skb);
- sk_ptr = skb->data;
- sk_len = skb->len;
+ dump_hdr = (struct qca_dump_hdr *)skb->data;
- if (pkt_type == HCI_ACLDATA_PKT) {
- sk_ptr += HCI_ACL_HDR_SIZE;
- sk_len -= HCI_ACL_HDR_SIZE;
- }
-
- sk_ptr += HCI_EVENT_HDR_SIZE;
- sk_len -= HCI_EVENT_HDR_SIZE;
-
- dump_hdr = (struct qca_dump_hdr *)sk_ptr;
seqno = le16_to_cpu(dump_hdr->seqno);
if (seqno == 0) {
set_bit(BTUSB_HW_SSR_ACTIVE, &btdata->flags);
@@ -3058,16 +3046,15 @@ static int handle_dump_pkt_qca(struct hci_dev *hdev, struct sk_buff *skb)
btdata->qca_dump.ram_dump_size = dump_size;
btdata->qca_dump.ram_dump_seqno = 0;
- sk_ptr += offsetof(struct qca_dump_hdr, data0);
- sk_len -= offsetof(struct qca_dump_hdr, data0);
+
+ skb_pull(skb, offsetof(struct qca_dump_hdr, data0));
usb_disable_autosuspend(udev);
bt_dev_info(hdev, "%s memdump size(%u)\n",
(pkt_type == HCI_ACLDATA_PKT) ? "ACL" : "event",
dump_size);
} else {
- sk_ptr += offsetof(struct qca_dump_hdr, data);
- sk_len -= offsetof(struct qca_dump_hdr, data);
+ skb_pull(skb, offsetof(struct qca_dump_hdr, data));
}
if (!btdata->qca_dump.ram_dump_size) {
@@ -3087,7 +3074,6 @@ static int handle_dump_pkt_qca(struct hci_dev *hdev, struct sk_buff *skb)
return ret;
}
- skb_pull(skb, skb->len - sk_len);
hci_devcd_append(hdev, skb);
btdata->qca_dump.ram_dump_seqno++;
if (seqno == QCA_LAST_SEQUENCE_NUM) {
@@ -3115,67 +3101,65 @@ static int handle_dump_pkt_qca(struct hci_dev *hdev, struct sk_buff *skb)
/* Return: true if the ACL packet is a dump packet, false otherwise. */
static bool acl_pkt_is_dump_qca(struct hci_dev *hdev, struct sk_buff *skb)
{
- u8 *sk_ptr;
- unsigned int sk_len;
-
struct hci_event_hdr *event_hdr;
struct hci_acl_hdr *acl_hdr;
struct qca_dump_hdr *dump_hdr;
+ void *orig_data;
+ unsigned int orig_len;
- sk_ptr = skb->data;
- sk_len = skb->len;
+ orig_data = skb->data;
+ orig_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;
+ acl_hdr = skb_pull_data(skb, sizeof(*acl_hdr));
+ if (!acl_hdr || (le16_to_cpu(acl_hdr->handle) != QCA_MEMDUMP_ACL_HANDLE))
+ goto restore_return;
- if ((event_hdr->evt != HCI_VENDOR_PKT)
- || (event_hdr->plen != (sk_len - HCI_EVENT_HDR_SIZE)))
- return false;
+ event_hdr = skb_pull_data(skb, sizeof(*event_hdr));
+ if (!event_hdr || (event_hdr->evt != HCI_VENDOR_PKT))
+ goto restore_return;
- 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;
+ dump_hdr = (struct qca_dump_hdr *)skb->data;
+ if ((skb->len < sizeof(*dump_hdr)) ||
+ (dump_hdr->vse_class != QCA_MEMDUMP_VSE_CLASS) ||
+ (dump_hdr->msg_type != QCA_MEMDUMP_MSG_TYPE))
+ goto restore_return;
return true;
+
+restore_return:
+ skb->data = orig_data;
+ skb->len = orig_len;
+ return false;
}
/* Return: true if the event packet is a dump packet, false otherwise. */
static bool evt_pkt_is_dump_qca(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;
+ void *orig_data;
+ unsigned int orig_len;
- sk_ptr = skb->data;
- sk_len = skb->len;
+ orig_data = skb->data;
+ orig_len = skb->len;
- event_hdr = hci_event_hdr(skb);
+ event_hdr = skb_pull_data(skb, sizeof(*event_hdr));
+ if (!event_hdr || (event_hdr->evt != HCI_VENDOR_PKT))
+ goto restore_return;
- if ((event_hdr->evt != HCI_VENDOR_PKT)
- || (event_hdr->plen != (sk_len - HCI_EVENT_HDR_SIZE)))
- return false;
+ dump_hdr = (struct qca_dump_hdr *)skb->data;
+ if ((skb->len < sizeof(*dump_hdr)) ||
+ (dump_hdr->vse_class != QCA_MEMDUMP_VSE_CLASS) ||
+ (dump_hdr->msg_type != QCA_MEMDUMP_MSG_TYPE))
+ goto restore_return;
- sk_ptr += HCI_EVENT_HDR_SIZE;
- sk_len -= HCI_EVENT_HDR_SIZE;
+ return true;
- 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;
+restore_return:
+ skb->data = orig_data;
+ skb->len = orig_len;
+ return false;
- return true;
}
static int btusb_recv_acl_qca(struct hci_dev *hdev, struct sk_buff *skb)
--
2.43.0
^ permalink raw reply related [flat|nested] 8+ messages in thread* Re: [PATCH v4 2/2] Bluetooth: btusb: use skb_pull to avoid unsafe access in QCA dump handling
2025-04-21 13:00 ` [PATCH v4 2/2] Bluetooth: btusb: use skb_pull to avoid unsafe access in QCA dump handling En-Wei Wu
@ 2025-04-21 15:29 ` Luiz Augusto von Dentz
0 siblings, 0 replies; 8+ messages in thread
From: Luiz Augusto von Dentz @ 2025-04-21 15:29 UTC (permalink / raw)
To: En-Wei Wu
Cc: marcel, linux-bluetooth, linux-kernel, pmenzel, quic_tjiang,
chia-lin.kao, anthony.wong
Hi En-Wei,
On Mon, Apr 21, 2025 at 9:00 AM En-Wei Wu <en-wei.wu@canonical.com> wrote:
>
> Use skb_pull() and skb_pull_data() to safely parse QCA dump packets.
>
> This avoids direct pointer math on skb->data, which could lead to
> invalid access if the packet is shorter than expected.
>
> Signed-off-by: En-Wei Wu <en-wei.wu@canonical.com>
> ---
> drivers/bluetooth/btusb.c | 98 ++++++++++++++++-----------------------
> 1 file changed, 41 insertions(+), 57 deletions(-)
>
> diff --git a/drivers/bluetooth/btusb.c b/drivers/bluetooth/btusb.c
> index f905780fcdea..031292ab766f 100644
> --- a/drivers/bluetooth/btusb.c
> +++ b/drivers/bluetooth/btusb.c
> @@ -3017,8 +3017,6 @@ static int handle_dump_pkt_qca(struct hci_dev *hdev, struct sk_buff *skb)
> {
> int ret = 0;
> u8 pkt_type;
> - u8 *sk_ptr;
> - unsigned int sk_len;
> u16 seqno;
> u32 dump_size;
>
> @@ -3027,18 +3025,8 @@ static int handle_dump_pkt_qca(struct hci_dev *hdev, struct sk_buff *skb)
> struct usb_device *udev = btdata->udev;
>
> pkt_type = hci_skb_pkt_type(skb);
> - sk_ptr = skb->data;
> - sk_len = skb->len;
> + dump_hdr = (struct qca_dump_hdr *)skb->data;
>
> - if (pkt_type == HCI_ACLDATA_PKT) {
> - sk_ptr += HCI_ACL_HDR_SIZE;
> - sk_len -= HCI_ACL_HDR_SIZE;
> - }
> -
> - sk_ptr += HCI_EVENT_HDR_SIZE;
> - sk_len -= HCI_EVENT_HDR_SIZE;
> -
> - dump_hdr = (struct qca_dump_hdr *)sk_ptr;
> seqno = le16_to_cpu(dump_hdr->seqno);
> if (seqno == 0) {
> set_bit(BTUSB_HW_SSR_ACTIVE, &btdata->flags);
> @@ -3058,16 +3046,15 @@ static int handle_dump_pkt_qca(struct hci_dev *hdev, struct sk_buff *skb)
>
> btdata->qca_dump.ram_dump_size = dump_size;
> btdata->qca_dump.ram_dump_seqno = 0;
> - sk_ptr += offsetof(struct qca_dump_hdr, data0);
> - sk_len -= offsetof(struct qca_dump_hdr, data0);
> +
> + skb_pull(skb, offsetof(struct qca_dump_hdr, data0));
>
> usb_disable_autosuspend(udev);
> bt_dev_info(hdev, "%s memdump size(%u)\n",
> (pkt_type == HCI_ACLDATA_PKT) ? "ACL" : "event",
> dump_size);
> } else {
> - sk_ptr += offsetof(struct qca_dump_hdr, data);
> - sk_len -= offsetof(struct qca_dump_hdr, data);
> + skb_pull(skb, offsetof(struct qca_dump_hdr, data));
> }
>
> if (!btdata->qca_dump.ram_dump_size) {
> @@ -3087,7 +3074,6 @@ static int handle_dump_pkt_qca(struct hci_dev *hdev, struct sk_buff *skb)
> return ret;
> }
>
> - skb_pull(skb, skb->len - sk_len);
> hci_devcd_append(hdev, skb);
> btdata->qca_dump.ram_dump_seqno++;
> if (seqno == QCA_LAST_SEQUENCE_NUM) {
> @@ -3115,67 +3101,65 @@ static int handle_dump_pkt_qca(struct hci_dev *hdev, struct sk_buff *skb)
> /* Return: true if the ACL packet is a dump packet, false otherwise. */
> static bool acl_pkt_is_dump_qca(struct hci_dev *hdev, struct sk_buff *skb)
> {
> - u8 *sk_ptr;
> - unsigned int sk_len;
> -
> struct hci_event_hdr *event_hdr;
> struct hci_acl_hdr *acl_hdr;
> struct qca_dump_hdr *dump_hdr;
> + void *orig_data;
> + unsigned int orig_len;
>
> - sk_ptr = skb->data;
> - sk_len = skb->len;
> + orig_data = skb->data;
> + orig_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;
> + acl_hdr = skb_pull_data(skb, sizeof(*acl_hdr));
> + if (!acl_hdr || (le16_to_cpu(acl_hdr->handle) != QCA_MEMDUMP_ACL_HANDLE))
> + goto restore_return;
>
> - if ((event_hdr->evt != HCI_VENDOR_PKT)
> - || (event_hdr->plen != (sk_len - HCI_EVENT_HDR_SIZE)))
> - return false;
> + event_hdr = skb_pull_data(skb, sizeof(*event_hdr));
> + if (!event_hdr || (event_hdr->evt != HCI_VENDOR_PKT))
> + goto restore_return;
>
> - 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;
> + dump_hdr = (struct qca_dump_hdr *)skb->data;
> + if ((skb->len < sizeof(*dump_hdr)) ||
> + (dump_hdr->vse_class != QCA_MEMDUMP_VSE_CLASS) ||
> + (dump_hdr->msg_type != QCA_MEMDUMP_MSG_TYPE))
> + goto restore_return;
>
> return true;
> +
> +restore_return:
> + skb->data = orig_data;
> + skb->len = orig_len;
> + return false;
> }
>
> /* Return: true if the event packet is a dump packet, false otherwise. */
> static bool evt_pkt_is_dump_qca(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;
> + void *orig_data;
> + unsigned int orig_len;
>
> - sk_ptr = skb->data;
> - sk_len = skb->len;
> + orig_data = skb->data;
> + orig_len = skb->len;
>
> - event_hdr = hci_event_hdr(skb);
> + event_hdr = skb_pull_data(skb, sizeof(*event_hdr));
> + if (!event_hdr || (event_hdr->evt != HCI_VENDOR_PKT))
> + goto restore_return;
>
> - if ((event_hdr->evt != HCI_VENDOR_PKT)
> - || (event_hdr->plen != (sk_len - HCI_EVENT_HDR_SIZE)))
> - return false;
> + dump_hdr = (struct qca_dump_hdr *)skb->data;
> + if ((skb->len < sizeof(*dump_hdr)) ||
> + (dump_hdr->vse_class != QCA_MEMDUMP_VSE_CLASS) ||
> + (dump_hdr->msg_type != QCA_MEMDUMP_MSG_TYPE))
> + goto restore_return;
>
> - sk_ptr += HCI_EVENT_HDR_SIZE;
> - sk_len -= HCI_EVENT_HDR_SIZE;
> + return true;
>
> - 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;
> +restore_return:
> + skb->data = orig_data;
> + skb->len = orig_len;
> + return false;
>
> - return true;
> }
>
> static int btusb_recv_acl_qca(struct hci_dev *hdev, struct sk_buff *skb)
> --
> 2.43.0
While I agree using the likes of skb_pull is a much safer way to parse
these packets Im not so sure it is safe to restore the skb->data and
skb->len like that, perhaps we should be using skb_clone and
skb_unclone for example.
--
Luiz Augusto von Dentz
^ permalink raw reply [flat|nested] 8+ messages in thread
* Re: [PATCH v4 0/2] btusb: fix NULL pointer dereference in QCA devcoredump handling
2025-04-21 13:00 [PATCH v4 0/2] btusb: fix NULL pointer dereference in QCA devcoredump handling En-Wei Wu
2025-04-21 13:00 ` [PATCH v4 1/2] Bluetooth: btusb: avoid NULL pointer dereference in skb_dequeue() En-Wei Wu
2025-04-21 13:00 ` [PATCH v4 2/2] Bluetooth: btusb: use skb_pull to avoid unsafe access in QCA dump handling En-Wei Wu
@ 2025-04-21 19:17 ` Luiz Augusto von Dentz
2025-04-24 1:46 ` En-Wei WU
2 siblings, 1 reply; 8+ messages in thread
From: Luiz Augusto von Dentz @ 2025-04-21 19:17 UTC (permalink / raw)
To: En-Wei Wu
Cc: marcel, linux-bluetooth, linux-kernel, pmenzel, quic_tjiang,
chia-lin.kao, anthony.wong
Hi En-Wei,
On Mon, Apr 21, 2025 at 9:00 AM En-Wei Wu <en-wei.wu@canonical.com> wrote:
>
> This patch series fixes a NULL pointer dereference in skb_dequeue()
> during QCA devcoredump handling, and adds some safety checks to make the
> parsing more robust.
While at it, please move this logic to qca specific file, there is no
reason for this logic to remain inside btusb.c
> The first patch fixes the logic bug where dump packets were mistakenly
> passed to hci_recv_frame() and freed prematurely. This was caused by
> handle_dump_pkt_qca() returning 0 even when the dump was successfully
> handled. It also refactors dump packet detection into separate helpers
> for ACL and event packets.
>
> The second patch adds bounds checks and replaces direct pointer access
> with skb_pull() and skb_pull_data() to avoid accessing invalid memory
> on malformed packets.
>
> Tested on WCN7851 (0489:e0f3) with devcoredump enabled. Crash no
> longer occurs and dumps are processed correctly.
>
> Changes in v4:
> - Fix unused variable error in the first patch
> - Refine commit messages
>
> Changes in v3:
> - Use skb_pull_data() for safe packet header access
> - Split dump packet detection into separate ACL and event helpers
>
> Changes in v2:
> - Fixed typo in the title
> - Re-flowed commit message line to fit 72 characters
> - Added blank line before btusb_recv_acl_qca()
>
> En-Wei Wu (2):
> Bluetooth: btusb: avoid NULL pointer dereference in skb_dequeue()
> Bluetooth: btusb: use skb_pull to avoid unsafe access in QCA dump
> handling
>
> drivers/bluetooth/btusb.c | 120 +++++++++++++++++++++++---------------
> 1 file changed, 74 insertions(+), 46 deletions(-)
>
> --
> 2.43.0
>
--
Luiz Augusto von Dentz
^ permalink raw reply [flat|nested] 8+ messages in thread
* Re: [PATCH v4 0/2] btusb: fix NULL pointer dereference in QCA devcoredump handling
2025-04-21 19:17 ` [PATCH v4 0/2] btusb: fix NULL pointer dereference in QCA devcoredump handling Luiz Augusto von Dentz
@ 2025-04-24 1:46 ` En-Wei WU
2025-04-24 1:49 ` En-Wei WU
0 siblings, 1 reply; 8+ messages in thread
From: En-Wei WU @ 2025-04-24 1:46 UTC (permalink / raw)
To: Luiz Augusto von Dentz
Cc: marcel, linux-bluetooth, linux-kernel, pmenzel, quic_tjiang,
chia-lin.kao, anthony.wong
Hi Luiz,
> While at it, please move this logic to qca specific file, there is no
> reason for this logic to remain inside btusb.c
I'll work on the v5 soon. Here is what I plan for v5:
[PATCH v4 1/3]: Move the device-core-dump logic of QCA from btusb.c to btqca.c
[PATCH v4 2/3]: Fix the original NULL pointer dereference as in [PATCH v3 1/2]
[PATCH v4 3/3]: Use skb_pull for safer skb access as in [PATCH v3
2/2], and use skb_clone to avoid directly resuming skb->data and
skb->len
Many thanks,
En-Wei.
On Tue, 22 Apr 2025 at 03:17, Luiz Augusto von Dentz
<luiz.dentz@gmail.com> wrote:
>
> Hi En-Wei,
>
> On Mon, Apr 21, 2025 at 9:00 AM En-Wei Wu <en-wei.wu@canonical.com> wrote:
> >
> > This patch series fixes a NULL pointer dereference in skb_dequeue()
> > during QCA devcoredump handling, and adds some safety checks to make the
> > parsing more robust.
>
> While at it, please move this logic to qca specific file, there is no
> reason for this logic to remain inside btusb.c
>
> > The first patch fixes the logic bug where dump packets were mistakenly
> > passed to hci_recv_frame() and freed prematurely. This was caused by
> > handle_dump_pkt_qca() returning 0 even when the dump was successfully
> > handled. It also refactors dump packet detection into separate helpers
> > for ACL and event packets.
> >
> > The second patch adds bounds checks and replaces direct pointer access
> > with skb_pull() and skb_pull_data() to avoid accessing invalid memory
> > on malformed packets.
> >
> > Tested on WCN7851 (0489:e0f3) with devcoredump enabled. Crash no
> > longer occurs and dumps are processed correctly.
> >
> > Changes in v4:
> > - Fix unused variable error in the first patch
> > - Refine commit messages
> >
> > Changes in v3:
> > - Use skb_pull_data() for safe packet header access
> > - Split dump packet detection into separate ACL and event helpers
> >
> > Changes in v2:
> > - Fixed typo in the title
> > - Re-flowed commit message line to fit 72 characters
> > - Added blank line before btusb_recv_acl_qca()
> >
> > En-Wei Wu (2):
> > Bluetooth: btusb: avoid NULL pointer dereference in skb_dequeue()
> > Bluetooth: btusb: use skb_pull to avoid unsafe access in QCA dump
> > handling
> >
> > drivers/bluetooth/btusb.c | 120 +++++++++++++++++++++++---------------
> > 1 file changed, 74 insertions(+), 46 deletions(-)
> >
> > --
> > 2.43.0
> >
>
>
> --
> Luiz Augusto von Dentz
^ permalink raw reply [flat|nested] 8+ messages in thread
* Re: [PATCH v4 0/2] btusb: fix NULL pointer dereference in QCA devcoredump handling
2025-04-24 1:46 ` En-Wei WU
@ 2025-04-24 1:49 ` En-Wei WU
0 siblings, 0 replies; 8+ messages in thread
From: En-Wei WU @ 2025-04-24 1:49 UTC (permalink / raw)
To: Luiz Augusto von Dentz
Cc: marcel, linux-bluetooth, linux-kernel, pmenzel, quic_tjiang,
chia-lin.kao, anthony.wong
> [PATCH v4 1/3]: Move the device-core-dump logic of QCA from btusb.c to btqca.c
> [PATCH v4 2/3]: Fix the original NULL pointer dereference as in [PATCH v3 1/2]
> [PATCH v4 3/3]: Use skb_pull for safer skb access as in [PATCH v3
> 2/2], and use skb_clone to avoid directly resuming skb->data and skb->len
Should be [PATCH v5 */3].
On Thu, 24 Apr 2025 at 09:46, En-Wei WU <en-wei.wu@canonical.com> wrote:
>
> Hi Luiz,
>
> > While at it, please move this logic to qca specific file, there is no
> > reason for this logic to remain inside btusb.c
> I'll work on the v5 soon. Here is what I plan for v5:
>
> [PATCH v4 1/3]: Move the device-core-dump logic of QCA from btusb.c to btqca.c
> [PATCH v4 2/3]: Fix the original NULL pointer dereference as in [PATCH v3 1/2]
> [PATCH v4 3/3]: Use skb_pull for safer skb access as in [PATCH v3
> 2/2], and use skb_clone to avoid directly resuming skb->data and
> skb->len
>
> Many thanks,
> En-Wei.
>
> On Tue, 22 Apr 2025 at 03:17, Luiz Augusto von Dentz
> <luiz.dentz@gmail.com> wrote:
> >
> > Hi En-Wei,
> >
> > On Mon, Apr 21, 2025 at 9:00 AM En-Wei Wu <en-wei.wu@canonical.com> wrote:
> > >
> > > This patch series fixes a NULL pointer dereference in skb_dequeue()
> > > during QCA devcoredump handling, and adds some safety checks to make the
> > > parsing more robust.
> >
> > While at it, please move this logic to qca specific file, there is no
> > reason for this logic to remain inside btusb.c
> >
> > > The first patch fixes the logic bug where dump packets were mistakenly
> > > passed to hci_recv_frame() and freed prematurely. This was caused by
> > > handle_dump_pkt_qca() returning 0 even when the dump was successfully
> > > handled. It also refactors dump packet detection into separate helpers
> > > for ACL and event packets.
> > >
> > > The second patch adds bounds checks and replaces direct pointer access
> > > with skb_pull() and skb_pull_data() to avoid accessing invalid memory
> > > on malformed packets.
> > >
> > > Tested on WCN7851 (0489:e0f3) with devcoredump enabled. Crash no
> > > longer occurs and dumps are processed correctly.
> > >
> > > Changes in v4:
> > > - Fix unused variable error in the first patch
> > > - Refine commit messages
> > >
> > > Changes in v3:
> > > - Use skb_pull_data() for safe packet header access
> > > - Split dump packet detection into separate ACL and event helpers
> > >
> > > Changes in v2:
> > > - Fixed typo in the title
> > > - Re-flowed commit message line to fit 72 characters
> > > - Added blank line before btusb_recv_acl_qca()
> > >
> > > En-Wei Wu (2):
> > > Bluetooth: btusb: avoid NULL pointer dereference in skb_dequeue()
> > > Bluetooth: btusb: use skb_pull to avoid unsafe access in QCA dump
> > > handling
> > >
> > > drivers/bluetooth/btusb.c | 120 +++++++++++++++++++++++---------------
> > > 1 file changed, 74 insertions(+), 46 deletions(-)
> > >
> > > --
> > > 2.43.0
> > >
> >
> >
> > --
> > Luiz Augusto von Dentz
^ permalink raw reply [flat|nested] 8+ messages in thread