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

[Neeraj Sanjay Kale]

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