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

[Neeraj Kale]

Hi Ali, Luiz

This v4 approach looks correct - header preserved in skb->data for hci_devcd_append() and dump aborted cleanly on the error path.

Reviewed-by: Neeraj Kale neeraj.sanjaykale@xxxxxxx

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 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@xxxxxxxxx>
> Link:
> https://lore.ke/
> rnel.org%2Flinux-
> bluetooth%2FAS4PR04MB9692EC13E3176B6D7525D097E7A72%40AS4PR04M
> B9692.eurprd04.prod.outlook.com%2F&data=05%7C02%7Cneeraj.sanjaykale
> %40nxp.com%7C6bcc0ce20725451b167f08defd666c70%7C686ea1d3bc2b4c6f
> a92cd99c5c301635%7C0%7C0%7C639226813511440640%7CUnknown%7CTW
> FpbGZsb3d8eyJFbXB0eU1hcGkiOnRydWUsIlYiOiIwLjAuMDAwMCIsIlAiOiJXaW
> 4zMiIsIkFOIjoiTWFpbCIsIldUIjoyfQ%3D%3D%7C0%7C%7C%7C&sdata=YympY
> uys6asrlyl5WTAQitOSv2dLGy6jf%2By0Usf%2BRqQ%3D&reserved=0
> Signed-off-by: Ali Ahmet Memis <ali@xxxxxxxxxxxxxx>
> ---
> 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.ke/
> rnel.org%2Flinux-bluetooth%2F20260818080104.563675-1-
> ali%40iusegentoo.com%2F&data=05%7C02%7Cneeraj.sanjaykale%40nxp.com
> %7C6bcc0ce20725451b167f08defd666c70%7C686ea1d3bc2b4c6fa92cd99c5c3
> 01635%7C0%7C0%7C639226813511473591%7CUnknown%7CTWFpbGZsb3d8
> eyJFbXB0eU1hcGkiOnRydWUsIlYiOiIwLjAuMDAwMCIsIlAiOiJXaW4zMiIsIkFOIj
> oiTWFpbCIsIldUIjoyfQ%3D%3D%7C0%7C%7C%7C&sdata=H27cFkiatrKCHHeu
> %2BWxL%2FoJwwj1mnO8tClM3KoP9TgE%3D&reserved=0
>
> 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.ke/
> rnel.org%2Fall%2F20260814081221.913676-1-
> ali%40iusegentoo.com%2F&data=05%7C02%7Cneeraj.sanjaykale%40nxp.com
> %7C6bcc0ce20725451b167f08defd666c70%7C686ea1d3bc2b4c6fa92cd99c5c3
> 01635%7C0%7C0%7C639226813511491888%7CUnknown%7CTWFpbGZsb3d8
> eyJFbXB0eU1hcGkiOnRydWUsIlYiOiIwLjAuMDAwMCIsIlAiOiJXaW4zMiIsIkFOIj
> oiTWFpbCIsIldUIjoyfQ%3D%3D%7C0%7C%7C%7C&sdata=jH6B1H5Yho2fVz7it
> 5H8k31dQGGcX%2BU2Zm5JOGG%2F7G8%3D&reserved=0
>
>  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


NXP Confidential