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

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

[Ali Ahmet Memis]

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.

Fixes: 1fcf216462ec ("Bluetooth: btnxpuart: Validate the FW dump header length")
Reported-by: Neeraj Kale <neeraj.sanjaykale@nxp.com>
Link: https://lore.kernel.org/linux-bluetooth/AS4PR04MB9692EC13E3176B6D7525D097E7A72@AS4PR04MB9692.eurprd04.prod.outlook.com/
Signed-off-by: Ali Ahmet Memis <ali@iusegentoo.com>
---
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");
 		goto free_skb;
 	}
-- 
2.55.0

RE: Bluetooth: btnxpuart: Keep FW dump header in coredump chunks

[bluez.test.bot]

[-- Attachment #1: Type: text/plain, Size: 1181 bytes --]

This is automated email and please do not reply to this email!

Dear submitter,

Thank you for submitting the patches to the linux bluetooth mailing list.
This is a CI test results with your patch series:
PW Link:https://patchwork.kernel.org/project/bluetooth/list/?series=1147625

---Test result---

Test Summary:
CheckPatch                    PASS      0.80 seconds
VerifyFixes                   PASS      0.14 seconds
VerifySignedoff               PASS      0.59 seconds
GitLint                       PASS      0.35 seconds
SubjectPrefix                 PASS      0.13 seconds
BuildKernel                   PASS      28.64 seconds
CheckAllWarning               PASS      31.09 seconds
CheckSparse                   PASS      29.56 seconds
BuildKernel32                 PASS      27.05 seconds
CheckKernelLLVM               SKIP      0.00 seconds
TestRunnerSetup               PASS      511.83 seconds
IncrementalBuild              PASS      25.96 seconds

Details
##############################
Test: CheckKernelLLVM - SKIP
Desc: Build kernel with LLVM + context analysis
Output:
Clang not found


https://github.com/bluez/bluetooth-next/pull/606

---
Regards,
Linux Bluetooth

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

[Neeraj Kale]

Hi Ali,

Thank you for the quick fix.

Reviewed-by: Neeraj Kale neeraj.sanjaykale@nxp.com

Thanks,
Neeraj


>
> 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.
>
> Fixes: 1fcf216462ec ("Bluetooth: btnxpuart: Validate the FW dump header
> length")
> Reported-by: Neeraj Kale <neeraj.sanjaykale@nxp.com>
> Link:
> https://lore.ke/
> rnel.org%2Flinux-
> bluetooth%2FAS4PR04MB9692EC13E3176B6D7525D097E7A72%40AS4PR04M
> B9692.eurprd04.prod.outlook.com%2F&data=05%7C02%7Cneeraj.sanjaykale
> %40nxp.com%7Ce5c09f2752bc463e224608defcfee41a%7C686ea1d3bc2b4c6f
> a92cd99c5c301635%7C0%7C0%7C639226368879983123%7CUnknown%7CTW
> FpbGZsb3d8eyJFbXB0eU1hcGkiOnRydWUsIlYiOiIwLjAuMDAwMCIsIlAiOiJXaW
> 4zMiIsIkFOIjoiTWFpbCIsIldUIjoyfQ%3D%3D%7C0%7C%7C%7C&sdata=mEFZvI
> Xr7Buf8x4p9NDDl3B1mJJR93pBKXZuARuRmX0%3D&reserved=0
> Signed-off-by: Ali Ahmet Memis <ali@iusegentoo.com>
> ---
> 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");
>                 goto free_skb;
>         }
> --
> 2.55.0


NXP Confidential

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

[Luiz Augusto von Dentz]

Hi Ali, Neeraj,

On Tue, Aug 18, 2026 at 4:01 AM Ali Ahmet Memis <ali@iusegentoo.com> 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 <neeraj.sanjaykale@nxp.com>
> Link: https://lore.kernel.org/linux-bluetooth/AS4PR04MB9692EC13E3176B6D7525D097E7A72@AS4PR04MB9692.eurprd04.prod.outlook.com/
> Signed-off-by: Ali Ahmet Memis <ali@iusegentoo.com>
> ---
> 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

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

[Ali Ahmet Memis]

Hi Luiz,

On Tue, Aug 18, 2026 at 11:51 AM Luiz Augusto von Dentz
<luiz.dentz@gmail.com> 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.

> 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.

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.

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@iusegentoo.com> wrote:
>
> Hi Luiz,
>
> On Tue, Aug 18, 2026 at 11:51 AM Luiz Augusto von Dentz
> <luiz.dentz@gmail.com> 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

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

[Ali Ahmet Memis]

Hi Luiz,

On Tue, Aug 18, 2026 at 12:59 PM Luiz Augusto von Dentz
<luiz.dentz@gmail.com> wrote:
>
> 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.

The kernel uses these two fields only for framing. It does not interpret
them as part of the dump format or validate their contents. It also does
not compare buf_len with the actual payload length.

This cannot be moved to userspace because the driver has no other way to
know where a dump starts and ends. Without seq_num and buf_len, it
cannot know when to call hci_devcd_init() and hci_devcd_complete().

I agree there is some overlap here, and I think this is also why a short
chunk should not be dropped silently.

> 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.

You're right. That statement was too broad. A chunk shorter than the
header is dropped, so the analyzer never sees it.

> 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.

I would go with the second option.

Appending it does not really leave the interpretation to userspace. Each
header tells the analyzer how many bytes belong to that chunk. If we add
a fragment without a header, all following headers are shifted and the
rest of the dump becomes difficult to parse.

Aborting seems like a better fit. The firmware should not generate a
chunk smaller than the header, so seeing one means something has already
gone wrong. We can no longer rely on the rest of the dump being
consistent.

hci_devcd_abort() keeps the data collected so far and marks the dump
with HCI_DEVCOREDUMP_ABORT in the State line. Userspace can then see
that the dump is truncated. hci_qca and hci_vhci already use it on
their error paths.

> 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.

I think aborting closes that gap. The kernel decides that a chunk too
short to contain a header is invalid, and userspace can see that the
dump was aborted instead of silently missing bytes.

Would you prefer this as a separate patch on top? This patch only
restores the header in the appended chunk, so its Fixes tag can stay
focused on that regression. Handling short chunks with an abort would
be a separate behavior change.

Thanks,
Ali

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

[Luiz Augusto von Dentz]

Hi Ali,

On Tue, Aug 18, 2026 at 3:30 PM Ali Ahmet Memis <ali@iusegentoo.com> wrote:
>
> Hi Luiz,
>
> On Tue, Aug 18, 2026 at 12:59 PM Luiz Augusto von Dentz
> <luiz.dentz@gmail.com> wrote:
> >
> > 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.
>
> The kernel uses these two fields only for framing. It does not interpret
> them as part of the dump format or validate their contents. It also does
> not compare buf_len with the actual payload length.
>
> This cannot be moved to userspace because the driver has no other way to
> know where a dump starts and ends. Without seq_num and buf_len, it
> cannot know when to call hci_devcd_init() and hci_devcd_complete().
>
> I agree there is some overlap here, and I think this is also why a short
> chunk should not be dropped silently.
>
> > 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.
>
> You're right. That statement was too broad. A chunk shorter than the
> header is dropped, so the analyzer never sees it.
>
> > 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.
>
> I would go with the second option.
>
> Appending it does not really leave the interpretation to userspace. Each
> header tells the analyzer how many bytes belong to that chunk. If we add
> a fragment without a header, all following headers are shifted and the
> rest of the dump becomes difficult to parse.
>
> Aborting seems like a better fit. The firmware should not generate a
> chunk smaller than the header, so seeing one means something has already
> gone wrong. We can no longer rely on the rest of the dump being
> consistent.
>
> hci_devcd_abort() keeps the data collected so far and marks the dump
> with HCI_DEVCOREDUMP_ABORT in the State line. Userspace can then see
> that the dump is truncated. hci_qca and hci_vhci already use it on
> their error paths.
>
> > 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.
>
> I think aborting closes that gap. The kernel decides that a chunk too
> short to contain a header is invalid, and userspace can see that the
> dump was aborted instead of silently missing bytes.
>
> Would you prefer this as a separate patch on top? This patch only
> restores the header in the appended chunk, so its Fixes tag can stay
> focused on that regression. Handling short chunks with an abort would
> be a separate behavior change.

Lets spin a new version aborting if the chunk is too short, I will
probably have it fixed up in place with the origina change since that
wasn't pulled yet so it makes it easier to not have multiple Fixes tag
applying one on top of another.

> Thanks,
> Ali



-- 
Luiz Augusto von Dentz