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

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

Posted by Ali Ahmet Memis 1 month, 2 weeks ago

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 v2] Bluetooth: btnxpuart: Validate the FW dump header length

Posted by Ali Ahmet Memis 1 month, 2 weeks ago

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

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

Posted by Luiz Augusto von Dentz 1 month, 2 weeks ago

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

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

Posted by Ali Ahmet Memis 1 month, 2 weeks ago

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.

Use skb_pull_data() to validate and pull the FW dump header before
accessing its fields. Warn and reject the chunk if the header is
truncated.

Fixes: 998e447f443f ("Bluetooth: btnxpuart: Add support for HCI coredump feature")
Signed-off-by: Ali Ahmet Memis <ali@iusegentoo.com>
---
v3: Use skb_pull_data() to validate and pull the FW dump header, as
    suggested by Luiz.
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 | 15 ++++++++++++---
 1 file changed, 12 insertions(+), 3 deletions(-)

diff --git a/drivers/bluetooth/btnxpuart.c b/drivers/bluetooth/btnxpuart.c
index 6a1cffe08d5f..e6c15bc6a30b 100644
--- a/drivers/bluetooth/btnxpuart.c
+++ b/drivers/bluetooth/btnxpuart.c
@@ -1368,12 +1368,21 @@ static int nxp_process_fw_dump(struct hci_dev *hdev, struct sk_buff *skb)
 {
 	struct hci_acl_hdr *acl_hdr = (struct hci_acl_hdr *)skb_pull_data(skb,
 									  sizeof(*acl_hdr));
-	struct nxp_fw_dump_hdr *fw_dump_hdr = (struct nxp_fw_dump_hdr *)skb->data;
+	struct nxp_fw_dump_hdr *fw_dump_hdr;
 	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;
 
+	fw_dump_hdr = skb_pull_data(skb, sizeof(*fw_dump_hdr));
+	if (!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