Re: [PATCH] Bluetooth: btnxpuart: Keep FW dump header in coredump chunks

Hi Ali, Neeraj,

On Tue, Aug 18, 2026 at 4:01 AM Ali Ahmet Memis <[email protected]> wrote:
>
> Commit 1fcf216462ec ("Bluetooth: btnxpuart: Validate the FW dump
> header length") changed nxp_process_fw_dump() to use skb_pull_data()
> when validating the FW dump header.
>
> This also removes the header from skb->data. Since the skb is cloned
> for hci_devcd_append() afterwards, the FW dump header is missing from
> the coredump chunk.
>
> The NXP FW dump analyzer expects nxp_fw_dump_hdr at the beginning of
> each chunk, so this results in invalid coredumps.
>
> Check the header length without pulling it from the skb instead. This
> keeps skb->data pointing to the FW dump header when the skb is cloned.

Ok, but if that is the case why are we parsing the headers in the
kernel? I thought the idea was that the kernel would assemble all the
segments and then push the dump as a whole. However, the above
suggests the NXP FW analyzer expects the headers. In that case I would
just have each segment reported on its own rather than appending it to
a separate skb including the headers.

> Fixes: 1fcf216462ec ("Bluetooth: btnxpuart: Validate the FW dump header length")
> Reported-by: Neeraj Kale <[email protected]>
> Link: https://lore.kernel.org/linux-bluetooth/AS4PR04MB9692EC13E3176B6D7525D097E7A72@AS4PR04MB9692.eurprd04.prod.outlook.com/
> Signed-off-by: Ali Ahmet Memis <[email protected]>
> ---
> Sorry for the late follow-up. I missed your reply on the v2 thread and
> only noticed it after the change had landed. This patch restores the
> approach from v2, as you suggested.
>
>  drivers/bluetooth/btnxpuart.c | 6 +++---
>  1 file changed, 3 insertions(+), 3 deletions(-)
>
> diff --git a/drivers/bluetooth/btnxpuart.c b/drivers/bluetooth/btnxpuart.c
> index f2bbe6e462aa..16d0f709d4b7 100644
> --- a/drivers/bluetooth/btnxpuart.c
> +++ b/drivers/bluetooth/btnxpuart.c
> @@ -1359,14 +1359,14 @@ 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 *fw_dump_hdr = (struct nxp_fw_dump_hdr *)skb->data;
>         struct btnxpuart_dev *nxpdev = hci_get_drvdata(hdev);
>         __u16 seq_num;
>         __u16 buf_len;
>         int err;
>
> -       fw_dump_hdr = skb_pull_data(skb, sizeof(*fw_dump_hdr));
> -       if (!fw_dump_hdr) {
> +       /* 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");

Id argue that this should be appended as is then, and the analyzer
should be the one checking it, _or_ it needs changing and then it only
process the dump _after_ reassemble.

>                 goto free_skb;
>         }
> --
> 2.55.0
>


-- 
Luiz Augusto von Dentz