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

[PATCH v4] 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
can therefore read both fields beyond the received data.

This is not only an out-of-bounds read. buf_len is also what terminates
a dump, so a value of zero makes the driver call hci_devcd_complete()
and reset the controller. A truncated frame can therefore end a dump
early.

Check that the payload is long enough before reading the header. The
header is not pulled from the skb because the skb is cloned for
hci_devcd_append() afterwards and the NXP FW dump analyzer expects
nxp_fw_dump_hdr at the beginning of each chunk in the dump.

Abort the dump when a chunk is too short to contain the header. The
firmware is not expected to generate such chunks, so receiving one
means something already went wrong and the rest of the dump can no
longer be trusted. Dropping it silently would leave userspace with a
dump that looks complete even though a chunk went missing from it.

hci_devcd_abort() still reports the data collected so far and records
HCI_DEVCOREDUMP_ABORT in the State line of the dump header, so userspace
can tell that the dump is truncated. The controller is reset as on the
completion path because BTNXPUART_FW_DUMP_IN_PROGRESS makes
nxp_enqueue() reject commands until the reset clears it.

Fixes: 998e447f443f ("Bluetooth: btnxpuart: Add support for HCI coredump feature")
Suggested-by: Luiz Augusto von Dentz <luiz.dentz@gmail.com>
Link: https://lore.kernel.org/linux-bluetooth/AS4PR04MB9692EC13E3176B6D7525D097E7A72@AS4PR04MB9692.eurprd04.prod.outlook.com/
Signed-off-by: Ali Ahmet Memis <ali@iusegentoo.com>
---
v4: Restore the v2 approach of checking the length without pulling the
    header. skb_pull_data() in v3 also advances skb->data, so the clone
    passed to hci_devcd_append() lost the nxp_fw_dump_hdr that the NXP
    dump analyzer expects, as Neeraj pointed out. On top of that, abort
    the dump when a chunk is too short, as suggested by Luiz.

    Sent as a new version rather than as an incremental fix on top of
    1fcf216462ec, since you mentioned folding it in. It applies to
    1fcf216462ec^. If you would rather keep 1fcf216462ec and take a
    delta on top, let me know and I will send that instead.

    Dropped Neeraj's Reviewed-by from v2 and from the follow-up patch,
    since the abort handling is new here.
    https://lore.kernel.org/linux-bluetooth/20260818080104.563675-1-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 | 17 +++++++++++++++--
 1 file changed, 15 insertions(+), 2 deletions(-)

diff --git a/drivers/bluetooth/btnxpuart.c b/drivers/bluetooth/btnxpuart.c
index e2b8f7997e4e..f6d08942a91e 100644
--- a/drivers/bluetooth/btnxpuart.c
+++ b/drivers/bluetooth/btnxpuart.c
@@ -1361,10 +1361,23 @@ 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");
+		if (fw_dump_in_progress(nxpdev)) {
+			hci_devcd_abort(hdev);
+			nxp_set_ind_reset(hdev, NULL);
+		}
+		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");

base-commit: c519ffc1e2c669296b976d11f5e7a79d2f82debb
-- 
2.55.0

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

[bluez.test.bot]

[-- Attachment #1: Type: text/plain, Size: 561 bytes --]

This is an automated email and please do not reply to this email.

Dear Submitter,

Thank you for submitting the patches to the linux bluetooth mailing list.
While preparing the CI tests, the patches you submitted couldn't be applied to the current HEAD of the repository.

----- Output -----

error: patch failed: drivers/bluetooth/btnxpuart.c:1361
error: drivers/bluetooth/btnxpuart.c: patch does not apply
hint: Use 'git am --show-current-patch' to see the failed patch

Please resolve the issue and submit the patches again.


---
Regards,
Linux Bluetooth