Re: [PATCH] Bluetooth: btnxpuart: Fix out-of-bounds firmware read in nxp_recv_fw_req_v1()

[Doruk (0sec)]

Hi Neeraj,

Thank you, and sorry if I'm missing sth!

That patchwork link points to Zhao
Dongdong's "Fix use-after-free in probe error path" patch, which addresses
the probe-path use-after-free rather than the firmware-download read.

On the firmware-read side: commit 25c286d75821 ("Bluetooth: btnxpuart: Fix
out-of-bounds firmware read in nxp_recv_fw_req_v3()") bounded the v3 handler.
My patch targets the v1 handler, nxp_recv_fw_req_v1(), where (at least in
today's bluetooth-next) the header length is still read at
nxp_get_data_len(nxpdev->fw->data + nxpdev->fw_dnld_v1_offset)

with no bound on fw_dnld_v1_offset, and the payload write still uses the
non-overflow-safe "fw_dnld_v1_offset + len <= fw->size" form
(fw_dnld_v1_offset is u32, so the sum can wrap).

If it's still open, I'm happy to send a v2 that also logs the
offset/size values,
as Paul suggested. Either way, thanks very much for taking the time,
and I'm happy to defer to whatever you have queued.

Best,
Doruk



On Mon, 6 Jul 2026 at 06:20, Neeraj Sanjay Kale
<neeraj.sanjaykale@xxxxxxx> wrote:
>
> Hi Doruk,
>
> Thank you for submitting this patch.
>
> However, a similar patch is already in review and approved by me:
> https://patchwork.kernel.org/project/bluetooth/patch/tencent_F2E2AF1B6F510577B10C6897ED768BBBAF07@xxxxxx/
> It's awaiting Luiz's review and/or merge.
>
>
> Hi Luiz,
>
> Can you please review the patch mentioned in the URL above, from Zhao Dongdong? I have answered your review comment.
> Thank you for your time and review.
>
> Thanks,
> Neeraj
>
>
> > Dear Doruk,
> >
> >
> > Thank you for the patch.
> >
> > Am 05.07.26 um 13:56 schrieb Doruk Tan Ozturk:
> > > Commit 25c286d75821 ("Bluetooth: btnxpuart: Fix out-of-bounds firmware
> > > read in nxp_recv_fw_req_v3()") bounded the v3 firmware download offset
> > > but left an unbounded read in the v1 handler.
> > >
> > > nxp_recv_fw_req_v1() advances a device-driven download offset
> > > (fw_dnld_v1_offset) by fw_v1_sent_bytes on every request, and that
> > > bookkeeping runs even when the payload write is skipped, so the offset
> > > can walk past nxpdev->fw->size. When the controller then requests a
> > > header (len == HDR_LEN), the driver reads the 16-byte bootloader
> > > header at
> > >
> > >    nxp_get_data_len(nxpdev->fw->data + nxpdev->fw_dnld_v1_offset)
> > >
> > > with no bound on the offset, reading past the end of the firmware image.
> > > A malicious or malfunctioning NXP UART controller can drive this to
> > > read out-of-bounds kernel memory during firmware download.
> > >
> > > Bound the offset before the header read, and convert the payload write
> > > guard to the overflow-safe form used by the v3 path (fw_dnld_v1_offset
> > > is u32, so fw_dnld_v1_offset + len can wrap).
> > >
> > > This was found by 0sec automated security-research tooling
> > >
> > (https://0sec.a/
> > i%2F&data=05%7C02%7Cneeraj.sanjaykale%40nxp.com%7Cc82fdb86e33f476
> > 570ed08dedad83110%7C686ea1d3bc2b4c6fa92cd99c5c301635%7C0%7C0%7
> > C639188819230990815%7CUnknown%7CTWFpbGZsb3d8eyJFbXB0eU1hcGkiO
> > nRydWUsIlYiOiIwLjAuMDAwMCIsIlAiOiJXaW4zMiIsIkFOIjoiTWFpbCIsIldUIjoyf
> > Q%3D%3D%7C0%7C%7C%7C&sdata=z6YC4OGfeSW45U2PbFFlFz13DG3%2FSr
> > qYeFKMSNTiMBI%3D&reserved=0).
> > >
> > > Fixes: 689ca16e5232 ("Bluetooth: NXP: Add protocol support for NXP
> > > Bluetooth chipsets")
> > > Cc: stable@xxxxxxxxxxxxxxx
> > > Assisted-by: 0sec:claude-opus-4-8
> > > Signed-off-by: Doruk Tan Ozturk <doruk@xxxxxxx>
> > > ---
> > >   drivers/bluetooth/btnxpuart.c | 13 ++++++++++---
> > >   1 file changed, 10 insertions(+), 3 deletions(-)
> > >
> > > diff --git a/drivers/bluetooth/btnxpuart.c
> > > b/drivers/bluetooth/btnxpuart.c index 6a1cffe08d5f..88d9ebf25a8f
> > > 100644
> > > --- a/drivers/bluetooth/btnxpuart.c
> > > +++ b/drivers/bluetooth/btnxpuart.c
> > > @@ -1041,11 +1041,17 @@ static int nxp_recv_fw_req_v1(struct hci_dev
> > *hdev, struct sk_buff *skb)
> > >                * and we need to re-send the previous header again.
> > >                */
> > >               if (len == nxpdev->fw_v1_expected_len) {
> > > -                     if (len == HDR_LEN)
> > > +                     if (len == HDR_LEN) {
> > > +                             if (nxpdev->fw_dnld_v1_offset >= nxpdev->fw->size ||
> > > +                                 nxpdev->fw->size - nxpdev->fw_dnld_v1_offset <
> > HDR_LEN) {
> > > +                                     bt_dev_err(hdev, "FW request
> > > + offset out of bounds");
> >
> > Would it make sense to log all the values, as I'd think, such an issue might be
> > hard to reproduce and gathering the values miht be difficult?
> >
> > > +                                     goto free_skb;
> > > +                             }
> > >                               nxpdev->fw_v1_expected_len = nxp_get_data_len(nxpdev-
> > >fw->data +
> > >                                                                       nxpdev->fw_dnld_v1_offset);
> > > -                     else
> > > +                     } else {
> > >                               nxpdev->fw_v1_expected_len = HDR_LEN;
> > > +                     }
> > >               } else if (len == HDR_LEN) {
> > >                       /* FW download out of sync. Send previous chunk again */
> > >                       nxpdev->fw_dnld_v1_offset -=
> > > nxpdev->fw_v1_sent_bytes; @@ -1053,7 +1059,8 @@ static int
> > nxp_recv_fw_req_v1(struct hci_dev *hdev, struct sk_buff *skb)
> > >               }
> > >       }
> > >
> > > -     if (nxpdev->fw_dnld_v1_offset + len <= nxpdev->fw->size)
> > > +     if (nxpdev->fw_dnld_v1_offset < nxpdev->fw->size &&
> > > +         len <= nxpdev->fw->size - nxpdev->fw_dnld_v1_offset)
> > >               serdev_device_write_buf(nxpdev->serdev, nxpdev->fw->data +
> > >                                       nxpdev->fw_dnld_v1_offset, len);
> > >       nxpdev->fw_v1_sent_bytes = len;
> >
> >
> > Kind regards,
> >
> > Paul