* [PATCH v3 0/2] Bluetooth: btusb: Fix QCA dump packet handling and improve SKB safety
@ 2024-12-05 7:17 En-Wei Wu
2024-12-05 7:17 ` [PATCH v3 1/2] Bluetooth: btusb: avoid NULL pointer dereference in skb_dequeue() En-Wei Wu
` (2 more replies)
0 siblings, 3 replies; 6+ messages in thread
From: En-Wei Wu @ 2024-12-05 7:17 UTC (permalink / raw)
To: marcel, luiz.dentz, linux-bluetooth, linux-kernel, pmenzel
Cc: quic_tjiang, kuan-ying.lee, anthony.wong
This patch series fixes a NULL pointer dereference in the QCA firmware dump
handling and improves the safety of SKB buffer handling. The problem occurs
when processing firmware crash dumps from WCN7851/WCN6855 Bluetooth
controllers, where incorrect return value handling leads to premature SKB
freeing and subsequent NULL pointer dereference.
The series is split into two parts:
- Patch 1 fixes the NULL pointer dereference by correcting return value
handling and splits dump packet detection into separate ACL and event
functions
- Patch 2 improves SKB safety by using proper buffer access methods and
adding state restoration on error paths
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: Improve SKB safety in QCA dump packet handling
drivers/bluetooth/btusb.c | 120 +++++++++++++++++++++++---------------
1 file changed, 74 insertions(+), 46 deletions(-)
--
2.43.0
^ permalink raw reply [flat|nested] 6+ messages in thread
* [PATCH v3 1/2] Bluetooth: btusb: avoid NULL pointer dereference in skb_dequeue()
2024-12-05 7:17 [PATCH v3 0/2] Bluetooth: btusb: Fix QCA dump packet handling and improve SKB safety En-Wei Wu
@ 2024-12-05 7:17 ` En-Wei Wu
2024-12-05 7:56 ` Bluetooth: btusb: Fix QCA dump packet handling and improve SKB safety bluez.test.bot
2024-12-05 7:17 ` [PATCH v3 2/2] Bluetooth: btusb: Improve SKB safety in QCA dump packet handling En-Wei Wu
2025-02-17 0:40 ` [PATCH v3 0/2] Bluetooth: btusb: Fix QCA dump packet handling and improve SKB safety Chia-Lin Kao (AceLan)
2 siblings, 1 reply; 6+ messages in thread
From: En-Wei Wu @ 2024-12-05 7:17 UTC (permalink / raw)
To: marcel, luiz.dentz, linux-bluetooth, linux-kernel, pmenzel
Cc: quic_tjiang, kuan-ying.lee, anthony.wong
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@canonical.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
^ permalink raw reply related [flat|nested] 6+ messages in thread
* [PATCH v3 2/2] Bluetooth: btusb: Improve SKB safety in QCA dump packet handling
2024-12-05 7:17 [PATCH v3 0/2] Bluetooth: btusb: Fix QCA dump packet handling and improve SKB safety En-Wei Wu
2024-12-05 7:17 ` [PATCH v3 1/2] Bluetooth: btusb: avoid NULL pointer dereference in skb_dequeue() En-Wei Wu
@ 2024-12-05 7:17 ` En-Wei Wu
2024-12-16 2:02 ` En-Wei WU
2025-02-17 0:40 ` [PATCH v3 0/2] Bluetooth: btusb: Fix QCA dump packet handling and improve SKB safety Chia-Lin Kao (AceLan)
2 siblings, 1 reply; 6+ messages in thread
From: En-Wei Wu @ 2024-12-05 7:17 UTC (permalink / raw)
To: marcel, luiz.dentz, linux-bluetooth, linux-kernel, pmenzel
Cc: quic_tjiang, kuan-ying.lee, anthony.wong
Replace direct buffer access and manual pointer arithmetic in QCA dump
packet handling with safer skb_pull_data() calls. This ensures proper
bounds checking when accessing packet headers and adds proper restoration
of SKB data pointer and length on error paths.
The changes include:
- Replacing manual pointer arithmetic with skb_pull_data() for
safer packet header access
- Adding SKB state restoration in error paths
This prevents potential buffer overflows and ensures SKB state remains
consistent even when packet validation fails.
Signed-off-by: En-Wei Wu <en-wei.wu@canonical.com>
---
drivers/bluetooth/btusb.c | 95 +++++++++++++++++----------------------
1 file changed, 41 insertions(+), 54 deletions(-)
diff --git a/drivers/bluetooth/btusb.c b/drivers/bluetooth/btusb.c
index 2bfb915062cf..cbeb1cec790a 100644
--- a/drivers/bluetooth/btusb.c
+++ b/drivers/bluetooth/btusb.c
@@ -2935,8 +2935,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;
@@ -2945,18 +2943,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);
@@ -2976,16 +2964,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) {
@@ -3005,7 +2992,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) {
@@ -3036,61 +3022,62 @@ 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;
+ 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;
-
- if ((event_hdr->evt != HCI_VENDOR_PKT)
- || (event_hdr->plen != (sk_len - HCI_EVENT_HDR_SIZE)))
- return false;
+ 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;
- sk_ptr += HCI_EVENT_HDR_SIZE;
- sk_len -= HCI_EVENT_HDR_SIZE;
+ event_hdr = skb_pull_data(skb, sizeof(*event_hdr));
+ if (!event_hdr || (event_hdr->evt != HCI_VENDOR_PKT))
+ goto restore_return;
- 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 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;
+ 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] 6+ messages in thread
* RE: Bluetooth: btusb: Fix QCA dump packet handling and improve SKB safety
2024-12-05 7:17 ` [PATCH v3 1/2] Bluetooth: btusb: avoid NULL pointer dereference in skb_dequeue() En-Wei Wu
@ 2024-12-05 7:56 ` bluez.test.bot
0 siblings, 0 replies; 6+ messages in thread
From: bluez.test.bot @ 2024-12-05 7:56 UTC (permalink / raw)
To: linux-bluetooth, en-wei.wu
[-- Attachment #1: Type: text/plain, Size: 2001 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=914799
---Test result---
Test Summary:
CheckPatch PENDING 0.26 seconds
GitLint PENDING 0.19 seconds
SubjectPrefix PASS 0.22 seconds
BuildKernel PASS 25.86 seconds
CheckAllWarning PASS 27.05 seconds
CheckSparse PASS 30.47 seconds
BuildKernel32 PASS 24.19 seconds
TestRunnerSetup PASS 434.56 seconds
TestRunner_l2cap-tester PASS 20.49 seconds
TestRunner_iso-tester FAIL 34.54 seconds
TestRunner_bnep-tester PASS 4.87 seconds
TestRunner_mgmt-tester PASS 121.33 seconds
TestRunner_rfcomm-tester PASS 7.74 seconds
TestRunner_sco-tester PASS 9.38 seconds
TestRunner_ioctl-tester PASS 8.20 seconds
TestRunner_mesh-tester PASS 6.11 seconds
TestRunner_smp-tester PASS 6.94 seconds
TestRunner_userchan-tester PASS 5.06 seconds
IncrementalBuild PENDING 0.39 seconds
Details
##############################
Test: CheckPatch - PENDING
Desc: Run checkpatch.pl script
Output:
##############################
Test: GitLint - PENDING
Desc: Run gitlint
Output:
##############################
Test: TestRunner_iso-tester - FAIL
Desc: Run iso-tester with test-runner
Output:
WARNING: possible circular locking dependency detected
Total: 125, Passed: 120 (96.0%), Failed: 1, Not Run: 4
Failed Test Cases
ISO Connect2 Suspend - Success Failed 4.258 seconds
##############################
Test: IncrementalBuild - PENDING
Desc: Incremental build with the patches in the series
Output:
---
Regards,
Linux Bluetooth
^ permalink raw reply [flat|nested] 6+ messages in thread
* Re: [PATCH v3 2/2] Bluetooth: btusb: Improve SKB safety in QCA dump packet handling
2024-12-05 7:17 ` [PATCH v3 2/2] Bluetooth: btusb: Improve SKB safety in QCA dump packet handling En-Wei Wu
@ 2024-12-16 2:02 ` En-Wei WU
0 siblings, 0 replies; 6+ messages in thread
From: En-Wei WU @ 2024-12-16 2:02 UTC (permalink / raw)
To: marcel, luiz.dentz, linux-bluetooth, linux-kernel, pmenzel
Cc: quic_tjiang, kuan-ying.lee, anthony.wong
Hi,
Sorry for bothering. May I kindly ask if there is any progress?
Thanks. Regards.
En-Wei.
On Thu, 5 Dec 2024 at 15:17, En-Wei Wu <en-wei.wu@canonical.com> wrote:
>
> Replace direct buffer access and manual pointer arithmetic in QCA dump
> packet handling with safer skb_pull_data() calls. This ensures proper
> bounds checking when accessing packet headers and adds proper restoration
> of SKB data pointer and length on error paths.
>
> The changes include:
> - Replacing manual pointer arithmetic with skb_pull_data() for
> safer packet header access
> - Adding SKB state restoration in error paths
>
> This prevents potential buffer overflows and ensures SKB state remains
> consistent even when packet validation fails.
>
> Signed-off-by: En-Wei Wu <en-wei.wu@canonical.com>
> ---
> drivers/bluetooth/btusb.c | 95 +++++++++++++++++----------------------
> 1 file changed, 41 insertions(+), 54 deletions(-)
>
> diff --git a/drivers/bluetooth/btusb.c b/drivers/bluetooth/btusb.c
> index 2bfb915062cf..cbeb1cec790a 100644
> --- a/drivers/bluetooth/btusb.c
> +++ b/drivers/bluetooth/btusb.c
> @@ -2935,8 +2935,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;
>
> @@ -2945,18 +2943,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);
> @@ -2976,16 +2964,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) {
> @@ -3005,7 +2992,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) {
> @@ -3036,61 +3022,62 @@ 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;
> + 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;
> -
> - if ((event_hdr->evt != HCI_VENDOR_PKT)
> - || (event_hdr->plen != (sk_len - HCI_EVENT_HDR_SIZE)))
> - return false;
> + 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;
>
> - sk_ptr += HCI_EVENT_HDR_SIZE;
> - sk_len -= HCI_EVENT_HDR_SIZE;
> + event_hdr = skb_pull_data(skb, sizeof(*event_hdr));
> + if (!event_hdr || (event_hdr->evt != HCI_VENDOR_PKT))
> + goto restore_return;
>
> - 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 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;
> + 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 [flat|nested] 6+ messages in thread
* Re: [PATCH v3 0/2] Bluetooth: btusb: Fix QCA dump packet handling and improve SKB safety
2024-12-05 7:17 [PATCH v3 0/2] Bluetooth: btusb: Fix QCA dump packet handling and improve SKB safety En-Wei Wu
2024-12-05 7:17 ` [PATCH v3 1/2] Bluetooth: btusb: avoid NULL pointer dereference in skb_dequeue() En-Wei Wu
2024-12-05 7:17 ` [PATCH v3 2/2] Bluetooth: btusb: Improve SKB safety in QCA dump packet handling En-Wei Wu
@ 2025-02-17 0:40 ` Chia-Lin Kao (AceLan)
2 siblings, 0 replies; 6+ messages in thread
From: Chia-Lin Kao (AceLan) @ 2025-02-17 0:40 UTC (permalink / raw)
To: En-Wei Wu
Cc: marcel, luiz.dentz, linux-bluetooth, linux-kernel, pmenzel,
quic_tjiang, kuan-ying.lee, anthony.wong
On Thu, Dec 05, 2024 at 03:17:25PM +0800, En-Wei Wu wrote:
> This patch series fixes a NULL pointer dereference in the QCA firmware dump
> handling and improves the safety of SKB buffer handling. The problem occurs
> when processing firmware crash dumps from WCN7851/WCN6855 Bluetooth
> controllers, where incorrect return value handling leads to premature SKB
> freeing and subsequent NULL pointer dereference.
A gentle ping.
Please help to review this patch series.
Thanks.
>
> The series is split into two parts:
> - Patch 1 fixes the NULL pointer dereference by correcting return value
> handling and splits dump packet detection into separate ACL and event
> functions
> - Patch 2 improves SKB safety by using proper buffer access methods and
> adding state restoration on error paths
>
> 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: Improve SKB safety in QCA dump packet handling
>
> drivers/bluetooth/btusb.c | 120 +++++++++++++++++++++++---------------
> 1 file changed, 74 insertions(+), 46 deletions(-)
>
> --
> 2.43.0
>
^ permalink raw reply [flat|nested] 6+ messages in thread
end of thread, other threads:[~2025-02-17 0:40 UTC | newest]
Thread overview: 6+ messages (download: mbox.gz follow: Atom feed
-- links below jump to the message on this page --
2024-12-05 7:17 [PATCH v3 0/2] Bluetooth: btusb: Fix QCA dump packet handling and improve SKB safety En-Wei Wu
2024-12-05 7:17 ` [PATCH v3 1/2] Bluetooth: btusb: avoid NULL pointer dereference in skb_dequeue() En-Wei Wu
2024-12-05 7:56 ` Bluetooth: btusb: Fix QCA dump packet handling and improve SKB safety bluez.test.bot
2024-12-05 7:17 ` [PATCH v3 2/2] Bluetooth: btusb: Improve SKB safety in QCA dump packet handling En-Wei Wu
2024-12-16 2:02 ` En-Wei WU
2025-02-17 0:40 ` [PATCH v3 0/2] Bluetooth: btusb: Fix QCA dump packet handling and improve SKB safety Chia-Lin Kao (AceLan)
This is an external index of several public inboxes,
see mirroring instructions on how to clone and mirror
all data and code used by this external index.