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

[Neeraj Sanjay Kale]

Hi Doruk,

Apologies for the confusion. I was referring to a different patch but looking more closely I realised both are different.

Apologies again.

Your patch tries to prevent an unbounded access in nxp_get_data_len() which seems genuine in a corrupt FW file or malicious controller.

Please do send a V2 patch as Paul requested with all offset, expected_len and size values logged, along with the reviewed-by tag.

Thanks,
Neeraj Kale

Reviewed-by: Neeraj Sanjay Kale <neeraj.sanjaykale@xxxxxxx>



> 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://patc/
> >
> hwork.kernel.org%2Fproject%2Fbluetooth%2Fpatch%2Ftencent_F2E2AF1B6F5
> 10
> >
> 577B10C6897ED768BBBAF07%40qq.com%2F&data=05%7C02%7Cneeraj.sanja
> ykale%4
> >
> 0nxp.com%7C667652efd25047817cd808deddfddf56%7C686ea1d3bc2b4c6fa9
> 2cd99c
> >
> 5c301635%7C0%7C0%7C639192279635192171%7CUnknown%7CTWFpbGZsb
> 3d8eyJFbXB0
> >
> eU1hcGkiOnRydWUsIlYiOiIwLjAuMDAwMCIsIlAiOiJXaW4zMiIsIkFOIjoiTWFpb
> CIsIl
> >
> dUIjoyfQ%3D%3D%7C0%7C%7C%7C&sdata=QYHW%2B%2BB2KeS1FlSJb36v4
> NHS1%2BLa8W
> > q0oQscOroZra0%3D&reserved=0 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://0.0.0.0/
> > >
> sec.a%2F&data=05%7C02%7Cneeraj.sanjaykale%40nxp.com%7C667652efd25
> 047
> > >
> 817cd808deddfddf56%7C686ea1d3bc2b4c6fa92cd99c5c301635%7C0%7C0%7
> C6391
> > >
> 92279635231913%7CUnknown%7CTWFpbGZsb3d8eyJFbXB0eU1hcGkiOnRyd
> WUsIlYiO
> > >
> iIwLjAuMDAwMCIsIlAiOiJXaW4zMiIsIkFOIjoiTWFpbCIsIldUIjoyfQ%3D%3D%7C
> 0%
> > >
> 7C%7C%7C&sdata=nLlRKyBusMXrHIK5NjCqZS7C0T6FTLfszBmagPI%2BDqQ%3
> D&rese
> > > rved=0
> > >
> 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