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

[Neeraj Kale]

Hi Luiz,

Thank you for the review comment.

Unfortunately, skb_pull_data() would advance skb->data past the dump header before hci_devcd_append() is called, so the cloned skb passed to it would be missing the nxp_fw_dump_hdr. The dump analyzer relies on those headers being present in the final .bin file, so this would silently corrupt the coredump output, breaking the NXP FW dump analyzer.

The explicit skb->len check in v2 avoids that — skb->data stays pointing at the header for the subsequent skb_clone(). Happy to keep it that way unless you see a cleaner alternative that preserves the header in the appended data.

Reviewed-by: Neeraj Kale neeraj.sanjaykale@xxxxxxx

Thanks again,
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>
> > ---
> > 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://eur01.safelinks.protection.outlook.com/?url=https%3A%2F%2Flore
> > .kernel.org%2Fall%2F20260814081221.913676-1-
> ali%40iusegentoo.com%2F&da
> >
> ta=05%7C02%7Cneeraj.sanjaykale%40nxp.com%7C7175c35558824341033408
> defa0
> >
> d80b5%7C686ea1d3bc2b4c6fa92cd99c5c301635%7C0%7C0%7C63922313306
> 8001419%
> >
> 7CUnknown%7CTWFpbGZsb3d8eyJFbXB0eU1hcGkiOnRydWUsIlYiOiIwLjAuM
> DAwMCIsIl
> >
> AiOiJXaW4zMiIsIkFOIjoiTWFpbCIsIldUIjoyfQ%3D%3D%7C0%7C%7C%7C&sdat
> a=uY2A
> > 2d6iXX4QItxfUKPivaK%2Fs%2Fn3WLl7TVyAW1sUehk%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 */
>
> 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");
> > --

NXP Confidential