[PATCH] Bluetooth: btnxpuart: Validate the FW dump header length

[PATCH] Bluetooth: btnxpuart: Validate the FW dump header length

[Ali Ahmet Memis]

nxp_process_fw_dump() pulls the ACL header off the frame and then reads
seq_num and buf_len from a struct nxp_fw_dump_hdr placed at skb->data,
without checking that the ACL payload is long enough to contain it.

h4_recv_buf() collects HCI_ACL_HDR_SIZE bytes of header followed by the
number of payload bytes named in that header, so skb->len is 4 + dlen
with dlen supplied by the controller and possibly smaller than the 8
byte dump header, or zero. A short frame with connection handle 0xfff
therefore reads both fields from beyond the received data.

Beyond the read itself, buf_len is what terminates a dump, a value of
zero makes the driver call hci_devcd_complete() and reset the
controller, so a truncated frame can end a dump early.

Reject frames whose payload is shorter than the dump header.

Fixes: 998e447f443f ("Bluetooth: btnxpuart: Add support for HCI coredump feature")
Signed-off-by: Ali Ahmet Memis <ali@iusegentoo.com>
---
 drivers/bluetooth/btnxpuart.c | 11 +++++++++--
 1 file changed, 9 insertions(+), 2 deletions(-)

diff --git a/drivers/bluetooth/btnxpuart.c b/drivers/bluetooth/btnxpuart.c
index 6a1cffe08d5f..e540acdb784a 100644
--- a/drivers/bluetooth/btnxpuart.c
+++ b/drivers/bluetooth/btnxpuart.c
@@ -1370,10 +1370,17 @@ static int nxp_process_fw_dump(struct hci_dev *hdev, struct sk_buff *skb)
 									  sizeof(*acl_hdr));
 	struct nxp_fw_dump_hdr *fw_dump_hdr = (struct nxp_fw_dump_hdr *)skb->data;
 	struct btnxpuart_dev *nxpdev = hci_get_drvdata(hdev);
-	__u16 seq_num = __le16_to_cpu(fw_dump_hdr->seq_num);
-	__u16 buf_len = __le16_to_cpu(fw_dump_hdr->buf_len);
+	__u16 seq_num;
+	__u16 buf_len;
 	int err;
 
+	/* The ACL payload must be long enough to hold the FW dump header */
+	if (skb->len < sizeof(*fw_dump_hdr))
+		goto free_skb;
+
+	seq_num = __le16_to_cpu(fw_dump_hdr->seq_num);
+	buf_len = __le16_to_cpu(fw_dump_hdr->buf_len);
+
 	if (seq_num == 0x0001) {
 		if (test_and_set_bit(BTNXPUART_FW_DUMP_IN_PROGRESS, &nxpdev->tx_state)) {
 			bt_dev_err(hdev, "FW dump already in progress");
-- 
2.55.0

[PATCH] Bluetooth: btnxpuart: Validate the FW dump header length

[Neeraj Kale]

Hi Ali,

Thank you for the patch!

The fix looks correct. One minor suggestion: add a log message on the early-exit path so a truncated frame doesn't fail silently:

bt_dev_warn(hdev, "FW dump: invalid or corrupt fw dump chunk\n");

Thanks,
Neeraj


> nxp_process_fw_dump() pulls the ACL header off the frame and then reads
> seq_num and buf_len from a struct nxp_fw_dump_hdr placed at skb->data,
> without checking that the ACL payload is long enough to contain it.
>
> h4_recv_buf() collects HCI_ACL_HDR_SIZE bytes of header followed by the
> number of payload bytes named in that header, so skb->len is 4 + dlen with
> dlen supplied by the controller and possibly smaller than the 8 byte dump
> header, or zero. A short frame with connection handle 0xfff therefore reads
> both fields from beyond the received data.
>
> Beyond the read itself, buf_len is what terminates a dump, a value of zero
> makes the driver call hci_devcd_complete() and reset the controller, so a
> truncated frame can end a dump early.
>
> Reject frames whose payload is shorter than the dump header.
>
> Fixes: 998e447f443f ("Bluetooth: btnxpuart: Add support for HCI coredump
> feature")
> Signed-off-by: Ali Ahmet Memis <ali@iusegentoo.com>
> ---
>  drivers/bluetooth/btnxpuart.c | 11 +++++++++--
>  1 file changed, 9 insertions(+), 2 deletions(-)
>
> diff --git a/drivers/bluetooth/btnxpuart.c b/drivers/bluetooth/btnxpuart.c
> index 6a1cffe08d5f..e540acdb784a 100644
> --- a/drivers/bluetooth/btnxpuart.c
> +++ b/drivers/bluetooth/btnxpuart.c
> @@ -1370,10 +1370,17 @@ static int nxp_process_fw_dump(struct hci_dev
> *hdev, struct sk_buff *skb)
>                                                                           sizeof(*acl_hdr));
>         struct nxp_fw_dump_hdr *fw_dump_hdr = (struct nxp_fw_dump_hdr
> *)skb->data;
>         struct btnxpuart_dev *nxpdev = hci_get_drvdata(hdev);
> -       __u16 seq_num = __le16_to_cpu(fw_dump_hdr->seq_num);
> -       __u16 buf_len = __le16_to_cpu(fw_dump_hdr->buf_len);
> +       __u16 seq_num;
> +       __u16 buf_len;
>         int err;
>
> +       /* The ACL payload must be long enough to hold the FW dump header
> */
> +       if (skb->len < sizeof(*fw_dump_hdr))
> +               goto free_skb;
> +
> +       seq_num = __le16_to_cpu(fw_dump_hdr->seq_num);
> +       buf_len = __le16_to_cpu(fw_dump_hdr->buf_len);
> +
>         if (seq_num == 0x0001) {
>                 if (test_and_set_bit(BTNXPUART_FW_DUMP_IN_PROGRESS,
> &nxpdev->tx_state)) {
>                         bt_dev_err(hdev, "FW dump already in progress");
> --
> 2.55.0


NXP Confidential

[PATCH v2] Bluetooth: btnxpuart: Validate the FW dump header length

[Ali Ahmet Memis]

nxp_process_fw_dump() pulls the ACL header off the frame and then reads
seq_num and buf_len from a struct nxp_fw_dump_hdr placed at skb->data,
without checking that the ACL payload is long enough to contain it.

h4_recv_buf() collects HCI_ACL_HDR_SIZE bytes of header followed by the
number of payload bytes named in that header, so skb->len is 4 + dlen
with dlen supplied by the controller and possibly smaller than the 8
byte dump header, or zero. A short frame with connection handle 0xfff
therefore reads both fields from beyond the received data.

Beyond the read itself, buf_len is what terminates a dump: a value of
zero makes the driver call hci_devcd_complete() and reset the
controller, so a truncated frame can end a dump early.

Reject frames whose payload is shorter than the dump header.

Fixes: 998e447f443f ("Bluetooth: btnxpuart: Add support for HCI coredump feature")
Signed-off-by: Ali Ahmet Memis <ali@iusegentoo.com>
---
v2: Warn on the early exit path instead of dropping the frame silently,
    as suggested by Neeraj. Dropped the trailing newline from the
    suggested message, since bt_dev_warn() already appends one.

v1: https://lore.kernel.org/all/20260814081221.913676-1-ali@iusegentoo.com/

 drivers/bluetooth/btnxpuart.c | 13 +++++++++++--
 1 file changed, 11 insertions(+), 2 deletions(-)

diff --git a/drivers/bluetooth/btnxpuart.c b/drivers/bluetooth/btnxpuart.c
index 6a1cffe08d5f..f439d287146e 100644
--- a/drivers/bluetooth/btnxpuart.c
+++ b/drivers/bluetooth/btnxpuart.c
@@ -1370,10 +1370,19 @@ static int nxp_process_fw_dump(struct hci_dev *hdev, struct sk_buff *skb)
 									  sizeof(*acl_hdr));
 	struct nxp_fw_dump_hdr *fw_dump_hdr = (struct nxp_fw_dump_hdr *)skb->data;
 	struct btnxpuart_dev *nxpdev = hci_get_drvdata(hdev);
-	__u16 seq_num = __le16_to_cpu(fw_dump_hdr->seq_num);
-	__u16 buf_len = __le16_to_cpu(fw_dump_hdr->buf_len);
+	__u16 seq_num;
+	__u16 buf_len;
 	int err;
 
+	/* The ACL payload must be long enough to hold the FW dump header */
+	if (skb->len < sizeof(*fw_dump_hdr)) {
+		bt_dev_warn(hdev, "FW dump: invalid or corrupt fw dump chunk");
+		goto free_skb;
+	}
+
+	seq_num = __le16_to_cpu(fw_dump_hdr->seq_num);
+	buf_len = __le16_to_cpu(fw_dump_hdr->buf_len);
+
 	if (seq_num == 0x0001) {
 		if (test_and_set_bit(BTNXPUART_FW_DUMP_IN_PROGRESS, &nxpdev->tx_state)) {
 			bt_dev_err(hdev, "FW dump already in progress");
-- 
2.55.0

[PATCH v2] Bluetooth: btnxpuart: Validate the FW dump header length

[Neeraj Kale]

Thank you for the patch again Ali.

The patch looks good to me.

Reviewed-by: Neeraj Sanjay Kale <neeraj.sanjaykale@nxp.com>

Thanks,
Neeraj

> nxp_process_fw_dump() pulls the ACL header off the frame and then reads
> seq_num and buf_len from a struct nxp_fw_dump_hdr placed at skb->data,
> without checking that the ACL payload is long enough to contain it.
>
> h4_recv_buf() collects HCI_ACL_HDR_SIZE bytes of header followed by the
> number of payload bytes named in that header, so skb->len is 4 + dlen with
> dlen supplied by the controller and possibly smaller than the 8 byte dump
> header, or zero. A short frame with connection handle 0xfff therefore reads
> both fields from beyond the received data.
>
> Beyond the read itself, buf_len is what terminates a dump: a value of zero
> makes the driver call hci_devcd_complete() and reset the controller, so a
> truncated frame can end a dump early.
>
> Reject frames whose payload is shorter than the dump header.
>
> Fixes: 998e447f443f ("Bluetooth: btnxpuart: Add support for HCI coredump
> feature")
> Signed-off-by: Ali Ahmet Memis <ali@iusegentoo.com>
> ---
> v2: Warn on the early exit path instead of dropping the frame silently,
>     as suggested by Neeraj. Dropped the trailing newline from the
>     suggested message, since bt_dev_warn() already appends one.
>
> v1:
> https://lore.ke/
> rnel.org%2Fall%2F20260814081221.913676-1-
> ali%40iusegentoo.com%2F&data=05%7C02%7Cneeraj.sanjaykale%40nxp.com
> %7C384b023f18684a2001ee08def9dfd3d3%7C686ea1d3bc2b4c6fa92cd99c5c
> 301635%7C0%7C0%7C639222936924705130%7CUnknown%7CTWFpbGZsb3d
> 8eyJFbXB0eU1hcGkiOnRydWUsIlYiOiIwLjAuMDAwMCIsIlAiOiJXaW4zMiIsIkFOI
> joiTWFpbCIsIldUIjoyfQ%3D%3D%7C0%7C%7C%7C&sdata=VVUw89WmnuOri
> XqyB5HlHgCIyBQOH7qBXxzucf4ikdI%3D&reserved=0
>
>  drivers/bluetooth/btnxpuart.c | 13 +++++++++++--
>  1 file changed, 11 insertions(+), 2 deletions(-)
>
> diff --git a/drivers/bluetooth/btnxpuart.c b/drivers/bluetooth/btnxpuart.c
> index 6a1cffe08d5f..f439d287146e 100644
> --- a/drivers/bluetooth/btnxpuart.c
> +++ b/drivers/bluetooth/btnxpuart.c
> @@ -1370,10 +1370,19 @@ static int nxp_process_fw_dump(struct hci_dev
> *hdev, struct sk_buff *skb)
>                                                                           sizeof(*acl_hdr));
>         struct nxp_fw_dump_hdr *fw_dump_hdr = (struct nxp_fw_dump_hdr
> *)skb->data;
>         struct btnxpuart_dev *nxpdev = hci_get_drvdata(hdev);
> -       __u16 seq_num = __le16_to_cpu(fw_dump_hdr->seq_num);
> -       __u16 buf_len = __le16_to_cpu(fw_dump_hdr->buf_len);
> +       __u16 seq_num;
> +       __u16 buf_len;
>         int err;
>
> +       /* The ACL payload must be long enough to hold the FW dump header
> */
> +       if (skb->len < sizeof(*fw_dump_hdr)) {
> +               bt_dev_warn(hdev, "FW dump: invalid or corrupt fw dump chunk");
> +               goto free_skb;
> +       }
> +
> +       seq_num = __le16_to_cpu(fw_dump_hdr->seq_num);
> +       buf_len = __le16_to_cpu(fw_dump_hdr->buf_len);
> +
>         if (seq_num == 0x0001) {
>                 if (test_and_set_bit(BTNXPUART_FW_DUMP_IN_PROGRESS,
> &nxpdev->tx_state)) {
>                         bt_dev_err(hdev, "FW dump already in progress");
> --
> 2.55.0


NXP Confidential

RE: [v2] Bluetooth: btnxpuart: Validate the FW dump header length

[bluez.test.bot]

[-- Attachment #1: Type: text/plain, Size: 1181 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=1145953

---Test result---

Test Summary:
CheckPatch                    PASS      8.64 seconds
VerifyFixes                   PASS      0.14 seconds
VerifySignedoff               PASS      0.14 seconds
GitLint                       PASS      0.34 seconds
SubjectPrefix                 PASS      0.13 seconds
BuildKernel                   PASS      27.85 seconds
CheckAllWarning               PASS      30.62 seconds
CheckSparse                   PASS      28.89 seconds
BuildKernel32                 PASS      26.76 seconds
CheckKernelLLVM               SKIP      0.00 seconds
TestRunnerSetup               PASS      506.96 seconds
IncrementalBuild              PASS      25.54 seconds

Details
##############################
Test: CheckKernelLLVM - SKIP
Desc: Build kernel with LLVM + context analysis
Output:
Clang not found


https://github.com/bluez/bluetooth-next/pull/581

---
Regards,
Linux Bluetooth

Re: [PATCH v2] Bluetooth: btnxpuart: Validate the FW dump header length

[Luiz Augusto von Dentz]

Hi Ali,

On Fri, Aug 14, 2026 at 4:41 AM Ali Ahmet Memis <ali@iusegentoo.com> wrote:
>
> nxp_process_fw_dump() pulls the ACL header off the frame and then reads
> seq_num and buf_len from a struct nxp_fw_dump_hdr placed at skb->data,
> without checking that the ACL payload is long enough to contain it.
>
> h4_recv_buf() collects HCI_ACL_HDR_SIZE bytes of header followed by the
> number of payload bytes named in that header, so skb->len is 4 + dlen
> with dlen supplied by the controller and possibly smaller than the 8
> byte dump header, or zero. A short frame with connection handle 0xfff
> therefore reads both fields from beyond the received data.
>
> Beyond the read itself, buf_len is what terminates a dump: a value of
> zero makes the driver call hci_devcd_complete() and reset the
> controller, so a truncated frame can end a dump early.
>
> Reject frames whose payload is shorter than the dump header.
>
> Fixes: 998e447f443f ("Bluetooth: btnxpuart: Add support for HCI coredump feature")
> Signed-off-by: Ali Ahmet Memis <ali@iusegentoo.com>
> ---
> v2: Warn on the early exit path instead of dropping the frame silently,
>     as suggested by Neeraj. Dropped the trailing newline from the
>     suggested message, since bt_dev_warn() already appends one.
>
> v1: https://lore.kernel.org/all/20260814081221.913676-1-ali@iusegentoo.com/
>
>  drivers/bluetooth/btnxpuart.c | 13 +++++++++++--
>  1 file changed, 11 insertions(+), 2 deletions(-)
>
> diff --git a/drivers/bluetooth/btnxpuart.c b/drivers/bluetooth/btnxpuart.c
> index 6a1cffe08d5f..f439d287146e 100644
> --- a/drivers/bluetooth/btnxpuart.c
> +++ b/drivers/bluetooth/btnxpuart.c
> @@ -1370,10 +1370,19 @@ static int nxp_process_fw_dump(struct hci_dev *hdev, struct sk_buff *skb)
>                                                                           sizeof(*acl_hdr));
>         struct nxp_fw_dump_hdr *fw_dump_hdr = (struct nxp_fw_dump_hdr *)skb->data;
>         struct btnxpuart_dev *nxpdev = hci_get_drvdata(hdev);
> -       __u16 seq_num = __le16_to_cpu(fw_dump_hdr->seq_num);
> -       __u16 buf_len = __le16_to_cpu(fw_dump_hdr->buf_len);
> +       __u16 seq_num;
> +       __u16 buf_len;
>         int err;
>
> +       /* The ACL payload must be long enough to hold the FW dump header */

The following can probably be replaced with skb_pull_data e.g:

fw_dump_hdr = skb_pull_data(skb, sizeof(*fw_dump_hdr));
if (!fw_dump_hdr)
...

> +       if (skb->len < sizeof(*fw_dump_hdr)) {
> +               bt_dev_warn(hdev, "FW dump: invalid or corrupt fw dump chunk");
> +               goto free_skb;
> +       }
> +
> +       seq_num = __le16_to_cpu(fw_dump_hdr->seq_num);
> +       buf_len = __le16_to_cpu(fw_dump_hdr->buf_len);
> +
>         if (seq_num == 0x0001) {
>                 if (test_and_set_bit(BTNXPUART_FW_DUMP_IN_PROGRESS, &nxpdev->tx_state)) {
>                         bt_dev_err(hdev, "FW dump already in progress");
> --
> 2.55.0
>


-- 
Luiz Augusto von Dentz