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

[Luiz Augusto von Dentz]

Hi Ali,

On Tue, Aug 18, 2026 at 12:25 PM Ali Ahmet Memis <ali@xxxxxxxxxxxxxx> wrote:
>
> Hi Luiz,
>
> On Tue, Aug 18, 2026 at 11:51 AM Luiz Augusto von Dentz
> <luiz.dentz@xxxxxxxxx> wrote:
> >
> > 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.
>
> The two fields are not just optional parsing for a concatenated
> dump. They are what the driver uses to handle the dump state.
>
> seq_num == 0x0001 tells us that a new dump has started, so
> hci_devcd_init() is called. buf_len == 0 marks the end of the dump,
> so hci_devcd_complete() is called.
>
> The chunks are passed to hci_devcd_append() as they
> arrive. nxp_process_fw_dump() does not check whether the dump is
> currently active before calling it. That state is handled by the
> coredump code itself.

Yeah, and it concatenates the header states into a dump skb,
interleaving data with headers. The worst part is that if the analyzer
finds an issue with the header it can consider the entire trace
malformed. This means both the kernel and userspace try to parse the
same thing but may interpret what each chunk means differently.

> > 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.
>
> The driver also does not otherwise interpret the chunk contents. It
> does not validate the payload length against buf_len, slice the
> payload, or remove anything from it. Once the chunk is long enough
> to contain the header, the whole skb is passed to hci_devcd_append()
> as-is, including the header.
>
> So the length check is only there to make reading seq_num and buf_len
> safe. It does not change what gets appended.

Well, it does change, it doesn't append anything if the chunk is less
than a header and that won't even show up for the analyzer to analyze.
Now, regarding this, I wonder if the driver receives a chunk smaller
than the header size, whether we should either append it and leave
interpretation to userspace, or 2 drop the entire dump because we
can't guarantee its consistency if we suspect data corruption when the
firmware is not supposed to generate chunks smaller than the header
size.

> The only packets we drop are those that are too short to contain
> the header. Without the header, there is no seq_num or buf_len,
> so we cannot determine whether the packet starts or ends a dump.

Yeah, the decision of considering a chunk valid or not seems to be
split between kernel and userspace. The kernel evaluates if the chunk
is too short but won't check any data consistency after that;
userspace will evaluate the headers but won't get a chance to evaluate
if any small chunk was dropped in the process.

-- 
Luiz Augusto von Dentz