[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@xxxxxxxxxxxxxx>
> ---
>  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