* [PATCH] Bluetooth: btusb: use skb_pull to avoid unsafe access in QCA dump handling
@ 2025-05-06 2:48 En-Wei Wu
2025-05-06 4:36 ` bluez.test.bot
2025-05-06 8:46 ` [PATCH] " Paul Menzel
0 siblings, 2 replies; 4+ messages in thread
From: En-Wei Wu @ 2025-05-06 2:48 UTC (permalink / raw)
To: marcel, luiz.dentz, linux-bluetooth, linux-kernel, pmenzel; +Cc: quic_tjiang
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 | 99 ++++++++++++++++-----------------------
1 file changed, 41 insertions(+), 58 deletions(-)
diff --git a/drivers/bluetooth/btusb.c b/drivers/bluetooth/btusb.c
index 357b18dae8de..17136924a278 100644
--- a/drivers/bluetooth/btusb.c
+++ b/drivers/bluetooth/btusb.c
@@ -2979,9 +2979,8 @@ static void btusb_coredump_qca(struct hci_dev *hdev)
static int handle_dump_pkt_qca(struct hci_dev *hdev, struct sk_buff *skb)
{
int ret = 0;
+ int skip = 0;
u8 pkt_type;
- u8 *sk_ptr;
- unsigned int sk_len;
u16 seqno;
u32 dump_size;
@@ -2990,18 +2989,14 @@ 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;
-
- if (pkt_type == HCI_ACLDATA_PKT) {
- sk_ptr += HCI_ACL_HDR_SIZE;
- sk_len -= HCI_ACL_HDR_SIZE;
- }
+ if (pkt_type == HCI_ACLDATA_PKT)
+ skip = sizeof(struct hci_acl_hdr) + sizeof(struct hci_event_hdr);
+ else
+ skip = sizeof(struct hci_event_hdr);
- sk_ptr += HCI_EVENT_HDR_SIZE;
- sk_len -= HCI_EVENT_HDR_SIZE;
+ skb_pull(skb, skip);
+ dump_hdr = (struct qca_dump_hdr *)skb->data;
- 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);
@@ -3021,16 +3016,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) {
@@ -3050,7 +3044,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) {
@@ -3078,68 +3071,58 @@ 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;
+ struct sk_buff *clone = skb_clone(skb, GFP_ATOMIC);
+ bool is_dump = false;
- 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)
+ if (!clone)
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(clone, sizeof(*acl_hdr));
+ if (!acl_hdr || (le16_to_cpu(acl_hdr->handle) != QCA_MEMDUMP_ACL_HANDLE))
+ goto out;
- sk_ptr += HCI_EVENT_HDR_SIZE;
- sk_len -= HCI_EVENT_HDR_SIZE;
+ event_hdr = skb_pull_data(clone, sizeof(*event_hdr));
+ if (!event_hdr || (event_hdr->evt != HCI_VENDOR_PKT))
+ goto out;
- 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 = skb_pull_data(clone, sizeof(*dump_hdr));
+ if (!dump_hdr || (dump_hdr->vse_class != QCA_MEMDUMP_VSE_CLASS) ||
+ (dump_hdr->msg_type != QCA_MEMDUMP_MSG_TYPE))
+ goto out;
- return true;
+ is_dump = true;
+out:
+ consume_skb(clone);
+ return is_dump;
}
/* 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;
+ struct sk_buff *clone = skb_clone(skb, GFP_ATOMIC);
+ bool is_dump = false;
- 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)))
+ if (!clone)
return false;
- sk_ptr += HCI_EVENT_HDR_SIZE;
- sk_len -= HCI_EVENT_HDR_SIZE;
+ event_hdr = skb_pull_data(clone, sizeof(*event_hdr));
+ if (!event_hdr || (event_hdr->evt != HCI_VENDOR_PKT))
+ goto out;
- 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 = skb_pull_data(clone, sizeof(*dump_hdr));
+ if (!dump_hdr || (dump_hdr->vse_class != QCA_MEMDUMP_VSE_CLASS) ||
+ (dump_hdr->msg_type != QCA_MEMDUMP_MSG_TYPE))
+ goto out;
- return true;
+ is_dump = true;
+out:
+ consume_skb(clone);
+ return is_dump;
}
static int btusb_recv_acl_qca(struct hci_dev *hdev, struct sk_buff *skb)
--
2.43.0
^ permalink raw reply related [flat|nested] 4+ messages in thread
* RE: Bluetooth: btusb: use skb_pull to avoid unsafe access in QCA dump handling
2025-05-06 2:48 [PATCH] Bluetooth: btusb: use skb_pull to avoid unsafe access in QCA dump handling En-Wei Wu
@ 2025-05-06 4:36 ` bluez.test.bot
2025-05-06 8:46 ` [PATCH] " Paul Menzel
1 sibling, 0 replies; 4+ messages in thread
From: bluez.test.bot @ 2025-05-06 4:36 UTC (permalink / raw)
To: linux-bluetooth, en-wei.wu
[-- Attachment #1: Type: text/plain, Size: 2029 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=959837
---Test result---
Test Summary:
CheckPatch PENDING 0.36 seconds
GitLint PENDING 0.23 seconds
SubjectPrefix PASS 0.58 seconds
BuildKernel PASS 24.54 seconds
CheckAllWarning PASS 26.50 seconds
CheckSparse PASS 30.16 seconds
BuildKernel32 PASS 23.80 seconds
TestRunnerSetup PASS 454.90 seconds
TestRunner_l2cap-tester PASS 24.33 seconds
TestRunner_iso-tester PASS 31.28 seconds
TestRunner_bnep-tester PASS 4.69 seconds
TestRunner_mgmt-tester FAIL 119.10 seconds
TestRunner_rfcomm-tester PASS 7.76 seconds
TestRunner_sco-tester PASS 12.90 seconds
TestRunner_ioctl-tester PASS 8.21 seconds
TestRunner_mesh-tester PASS 5.97 seconds
TestRunner_smp-tester PASS 7.06 seconds
TestRunner_userchan-tester PASS 5.26 seconds
IncrementalBuild PENDING 0.69 seconds
Details
##############################
Test: CheckPatch - PENDING
Desc: Run checkpatch.pl script
Output:
##############################
Test: GitLint - PENDING
Desc: Run gitlint
Output:
##############################
Test: TestRunner_mgmt-tester - FAIL
Desc: Run mgmt-tester with test-runner
Output:
Total: 490, Passed: 484 (98.8%), Failed: 2, Not Run: 4
Failed Test Cases
LL Privacy - Add Device 3 (AL is full) Failed 0.234 seconds
LL Privacy - Set Flags 1 (Add to RL) Failed 0.135 seconds
##############################
Test: IncrementalBuild - PENDING
Desc: Incremental build with the patches in the series
Output:
---
Regards,
Linux Bluetooth
^ permalink raw reply [flat|nested] 4+ messages in thread
* Re: [PATCH] Bluetooth: btusb: use skb_pull to avoid unsafe access in QCA dump handling
2025-05-06 2:48 [PATCH] Bluetooth: btusb: use skb_pull to avoid unsafe access in QCA dump handling En-Wei Wu
2025-05-06 4:36 ` bluez.test.bot
@ 2025-05-06 8:46 ` Paul Menzel
2025-05-07 5:29 ` En-Wei WU
1 sibling, 1 reply; 4+ messages in thread
From: Paul Menzel @ 2025-05-06 8:46 UTC (permalink / raw)
To: En-Wei Wu; +Cc: marcel, luiz.dentz, linux-bluetooth, linux-kernel, quic_tjiang
Dear En-Wei,
Thank you for your patch.
Am 06.05.25 um 04:48 schrieb En-Wei Wu:
> 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.
Please add a Fixes: tag.
Also, how did you test this?
> Signed-off-by: En-Wei Wu <en-wei.wu@canonical.com>
> ---
> drivers/bluetooth/btusb.c | 99 ++++++++++++++++-----------------------
> 1 file changed, 41 insertions(+), 58 deletions(-)
>
> diff --git a/drivers/bluetooth/btusb.c b/drivers/bluetooth/btusb.c
> index 357b18dae8de..17136924a278 100644
> --- a/drivers/bluetooth/btusb.c
> +++ b/drivers/bluetooth/btusb.c
> @@ -2979,9 +2979,8 @@ static void btusb_coredump_qca(struct hci_dev *hdev)
> static int handle_dump_pkt_qca(struct hci_dev *hdev, struct sk_buff *skb)
> {
> int ret = 0;
> + int skip = 0;
`unsigned int`, as the signature is:
include/linux/skbuff.h:void *skb_pull(struct sk_buff *skb, unsigned
int len);
> u8 pkt_type;
> - u8 *sk_ptr;
> - unsigned int sk_len;
> u16 seqno;
> u32 dump_size;
>
> @@ -2990,18 +2989,14 @@ 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;
> -
> - if (pkt_type == HCI_ACLDATA_PKT) {
> - sk_ptr += HCI_ACL_HDR_SIZE;
> - sk_len -= HCI_ACL_HDR_SIZE;
> - }
> + if (pkt_type == HCI_ACLDATA_PKT)
> + skip = sizeof(struct hci_acl_hdr) + sizeof(struct hci_event_hdr);
> + else
> + skip = sizeof(struct hci_event_hdr);
Maybe write it as below:
skip = sizeof(struct hci_event_hdr);
if (pkt_type == HCI_ACLDATA_PKT)
skip += sizeof(struct hci_acl_hdr);
Kind regards,
Paul
>
> - sk_ptr += HCI_EVENT_HDR_SIZE;
> - sk_len -= HCI_EVENT_HDR_SIZE;
> + skb_pull(skb, skip);
> + dump_hdr = (struct qca_dump_hdr *)skb->data;
>
> - 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);
> @@ -3021,16 +3016,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) {
> @@ -3050,7 +3044,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) {
> @@ -3078,68 +3071,58 @@ 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;
> + struct sk_buff *clone = skb_clone(skb, GFP_ATOMIC);
> + bool is_dump = false;
>
> - 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)
> + if (!clone)
> 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(clone, sizeof(*acl_hdr));
> + if (!acl_hdr || (le16_to_cpu(acl_hdr->handle) != QCA_MEMDUMP_ACL_HANDLE))
> + goto out;
>
> - sk_ptr += HCI_EVENT_HDR_SIZE;
> - sk_len -= HCI_EVENT_HDR_SIZE;
> + event_hdr = skb_pull_data(clone, sizeof(*event_hdr));
> + if (!event_hdr || (event_hdr->evt != HCI_VENDOR_PKT))
> + goto out;
>
> - 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 = skb_pull_data(clone, sizeof(*dump_hdr));
> + if (!dump_hdr || (dump_hdr->vse_class != QCA_MEMDUMP_VSE_CLASS) ||
> + (dump_hdr->msg_type != QCA_MEMDUMP_MSG_TYPE))
> + goto out;
>
> - return true;
> + is_dump = true;
> +out:
> + consume_skb(clone);
> + return is_dump;
> }
>
> /* 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;
> + struct sk_buff *clone = skb_clone(skb, GFP_ATOMIC);
> + bool is_dump = false;
>
> - 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)))
> + if (!clone)
> return false;
>
> - sk_ptr += HCI_EVENT_HDR_SIZE;
> - sk_len -= HCI_EVENT_HDR_SIZE;
> + event_hdr = skb_pull_data(clone, sizeof(*event_hdr));
> + if (!event_hdr || (event_hdr->evt != HCI_VENDOR_PKT))
> + goto out;
>
> - 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 = skb_pull_data(clone, sizeof(*dump_hdr));
> + if (!dump_hdr || (dump_hdr->vse_class != QCA_MEMDUMP_VSE_CLASS) ||
> + (dump_hdr->msg_type != QCA_MEMDUMP_MSG_TYPE))
> + goto out;
>
> - return true;
> + is_dump = true;
> +out:
> + consume_skb(clone);
> + return is_dump;
> }
>
> static int btusb_recv_acl_qca(struct hci_dev *hdev, struct sk_buff *skb)
^ permalink raw reply [flat|nested] 4+ messages in thread
* Re: [PATCH] Bluetooth: btusb: use skb_pull to avoid unsafe access in QCA dump handling
2025-05-06 8:46 ` [PATCH] " Paul Menzel
@ 2025-05-07 5:29 ` En-Wei WU
0 siblings, 0 replies; 4+ messages in thread
From: En-Wei WU @ 2025-05-07 5:29 UTC (permalink / raw)
To: Paul Menzel
Cc: marcel, luiz.dentz, linux-bluetooth, linux-kernel, quic_tjiang
Hi Paul,
> Also, how did you test this?
I triggered the device coredump by `$ echo 1` to the file named
"coredump" in the sysfs device node of the hci device. The symbolic
link of the file can be found at
/sys/class/bluetooth/hci*/device/coredump.
After triggering the coredump, the core dump file can be found at
/sys/class/devcoredump.
Kind regards,
En-Wei
On Tue, 6 May 2025 at 16:46, Paul Menzel <pmenzel@molgen.mpg.de> wrote:
>
> Dear En-Wei,
>
>
> Thank you for your patch.
>
> Am 06.05.25 um 04:48 schrieb En-Wei Wu:
> > 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.
>
> Please add a Fixes: tag.
>
> Also, how did you test this?
>
> > Signed-off-by: En-Wei Wu <en-wei.wu@canonical.com>
> > ---
> > drivers/bluetooth/btusb.c | 99 ++++++++++++++++-----------------------
> > 1 file changed, 41 insertions(+), 58 deletions(-)
> >
> > diff --git a/drivers/bluetooth/btusb.c b/drivers/bluetooth/btusb.c
> > index 357b18dae8de..17136924a278 100644
> > --- a/drivers/bluetooth/btusb.c
> > +++ b/drivers/bluetooth/btusb.c
> > @@ -2979,9 +2979,8 @@ static void btusb_coredump_qca(struct hci_dev *hdev)
> > static int handle_dump_pkt_qca(struct hci_dev *hdev, struct sk_buff *skb)
> > {
> > int ret = 0;
> > + int skip = 0;
>
> `unsigned int`, as the signature is:
>
> include/linux/skbuff.h:void *skb_pull(struct sk_buff *skb, unsigned
> int len);
>
> > u8 pkt_type;
> > - u8 *sk_ptr;
> > - unsigned int sk_len;
> > u16 seqno;
> > u32 dump_size;
> >
> > @@ -2990,18 +2989,14 @@ 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;
> > -
> > - if (pkt_type == HCI_ACLDATA_PKT) {
> > - sk_ptr += HCI_ACL_HDR_SIZE;
> > - sk_len -= HCI_ACL_HDR_SIZE;
> > - }
> > + if (pkt_type == HCI_ACLDATA_PKT)
> > + skip = sizeof(struct hci_acl_hdr) + sizeof(struct hci_event_hdr);
> > + else
> > + skip = sizeof(struct hci_event_hdr);
>
> Maybe write it as below:
>
> skip = sizeof(struct hci_event_hdr);
>
> if (pkt_type == HCI_ACLDATA_PKT)
> skip += sizeof(struct hci_acl_hdr);
>
>
> Kind regards,
>
> Paul
>
>
> >
> > - sk_ptr += HCI_EVENT_HDR_SIZE;
> > - sk_len -= HCI_EVENT_HDR_SIZE;
> > + skb_pull(skb, skip);
> > + dump_hdr = (struct qca_dump_hdr *)skb->data;
> >
> > - 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);
> > @@ -3021,16 +3016,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) {
> > @@ -3050,7 +3044,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) {
> > @@ -3078,68 +3071,58 @@ 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;
> > + struct sk_buff *clone = skb_clone(skb, GFP_ATOMIC);
> > + bool is_dump = false;
> >
> > - 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)
> > + if (!clone)
> > 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(clone, sizeof(*acl_hdr));
> > + if (!acl_hdr || (le16_to_cpu(acl_hdr->handle) != QCA_MEMDUMP_ACL_HANDLE))
> > + goto out;
> >
> > - sk_ptr += HCI_EVENT_HDR_SIZE;
> > - sk_len -= HCI_EVENT_HDR_SIZE;
> > + event_hdr = skb_pull_data(clone, sizeof(*event_hdr));
> > + if (!event_hdr || (event_hdr->evt != HCI_VENDOR_PKT))
> > + goto out;
> >
> > - 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 = skb_pull_data(clone, sizeof(*dump_hdr));
> > + if (!dump_hdr || (dump_hdr->vse_class != QCA_MEMDUMP_VSE_CLASS) ||
> > + (dump_hdr->msg_type != QCA_MEMDUMP_MSG_TYPE))
> > + goto out;
> >
> > - return true;
> > + is_dump = true;
> > +out:
> > + consume_skb(clone);
> > + return is_dump;
> > }
> >
> > /* 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;
> > + struct sk_buff *clone = skb_clone(skb, GFP_ATOMIC);
> > + bool is_dump = false;
> >
> > - 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)))
> > + if (!clone)
> > return false;
> >
> > - sk_ptr += HCI_EVENT_HDR_SIZE;
> > - sk_len -= HCI_EVENT_HDR_SIZE;
> > + event_hdr = skb_pull_data(clone, sizeof(*event_hdr));
> > + if (!event_hdr || (event_hdr->evt != HCI_VENDOR_PKT))
> > + goto out;
> >
> > - 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 = skb_pull_data(clone, sizeof(*dump_hdr));
> > + if (!dump_hdr || (dump_hdr->vse_class != QCA_MEMDUMP_VSE_CLASS) ||
> > + (dump_hdr->msg_type != QCA_MEMDUMP_MSG_TYPE))
> > + goto out;
> >
> > - return true;
> > + is_dump = true;
> > +out:
> > + consume_skb(clone);
> > + return is_dump;
> > }
> >
> > static int btusb_recv_acl_qca(struct hci_dev *hdev, struct sk_buff *skb)
>
^ permalink raw reply [flat|nested] 4+ messages in thread
end of thread, other threads:[~2025-05-07 5:29 UTC | newest]
Thread overview: 4+ messages (download: mbox.gz follow: Atom feed
-- links below jump to the message on this page --
2025-05-06 2:48 [PATCH] Bluetooth: btusb: use skb_pull to avoid unsafe access in QCA dump handling En-Wei Wu
2025-05-06 4:36 ` bluez.test.bot
2025-05-06 8:46 ` [PATCH] " Paul Menzel
2025-05-07 5:29 ` En-Wei WU
This is a public inbox, see mirroring instructions
for how to clone and mirror all data and code used for this inbox