btnxpuart: out of bounds read of firmware in nxp_recv_fw_req_v3()

[Neeraj Sanjay Kale]

Hi Maoyi,

You're right, the bug is real. The V3 handler is missing the bounds check that V1 already has, somehow missed it when V3 support was added.

Your fix is correct and in the right place and it won't break anything on a well-behaved chip.

One minor suggestion:
free_skb unconditionally saves len into fw_v3_prev_sent even on your new error path. Harmless, but you could zero len before the goto to keep it clean.

Please go ahead and send the patch. Thanks for the thorough report.

Regards,
Neeraj

> Hi all,
> 
> I think nxp_recv_fw_req_v3() in drivers/bluetooth/btnxpuart.c can read past
> the end of the firmware image. I would appreciate it if you could take a look.
> 
> The chip's v3 firmware download request carries a u32 offset and a u16 len.
> The handler checks only the lower bound, then sends firmware from that
> offset.
> 
>         offset = __le32_to_cpu(req->offset);
>         if (offset < nxpdev->fw_v3_offset_correction) {
>                 ... goto free_skb;
>         }
>         nxpdev->fw_dnld_v3_offset = offset - nxpdev->fw_v3_offset_correction;
>         serdev_device_write_buf(nxpdev->serdev,
>                 nxpdev->fw->data + nxpdev->fw_dnld_v3_offset, len);
> 
> Nothing checks that fw_dnld_v3_offset + len stays within nxpdev->fw->size.
> So a chip that asks for an offset or len past the firmware image makes the
> kernel read past nxpdev->fw->data and send that memory back to the chip
> over UART.
> 
> The v1 handler already bounds this. nxp_recv_fw_req_v1() guards the same
> write.
> 
>         if (nxpdev->fw_dnld_v1_offset + len <= nxpdev->fw->size)
>                 serdev_device_write_buf(...);
> 
> The v3 handler is missing that check.
> 
> I reproduced the read on 7.1-rc7 by replaying the serdev_device_write_buf()
> source read, with the firmware image ending at an unmapped page.
> 
>   BUG: unable to handle page fault for address: ...
>   #PF: error_code(0x0000) - not-present page      (a read)
>   RIP: 0010:... (the fw->data + offset read)
> 
> The fix I tried bounds the request against the image size, like the v1 path
> does.
> 
>                 nxpdev->fw_dnld_v3_offset = offset - nxpdev-
> >fw_v3_offset_correction;
>         +       if (nxpdev->fw_dnld_v3_offset >= nxpdev->fw->size ||
>         +           len > nxpdev->fw->size - nxpdev->fw_dnld_v3_offset) {
>         +               bt_dev_err(hdev, "FW download out of range");
>         +               goto free_skb;
>         +       }
>                 serdev_device_write_buf(nxpdev->serdev,
>                         nxpdev->fw->data + nxpdev->fw_dnld_v3_offset, len);
> 
> This needs a misbehaving controller, not a remote attacker. It still looks like an
> out of bounds read to me. Does this look right, and is that the place to bound
> it? Happy to send a proper patch once you confirm.
> 
> Thanks,
> Maoyi
> https://eur01.safelinks.protection.outlook.com/?url=https%3A%2F%2Fmaoyix
> ie.com%2F&data=05%7C02%7Cneeraj.sanjaykale%40nxp.com%7C84ae8fe300
> 0c4f0f7e6108decb7bdd15%7C686ea1d3bc2b4c6fa92cd99c5c301635%7C0%7C
> 1%7C639171930037825817%7CUnknown%7CTWFpbGZsb3d8eyJFbXB0eU1hc
> GkiOnRydWUsIlYiOiIwLjAuMDAwMCIsIlAiOiJXaW4zMiIsIkFOIjoiTWFpbCIsIldU
> IjoyfQ%3D%3D%7C80000%7C%7C%7C&sdata=LWulFK7EnsQarLkCSA%2Fsedu
> 3X015VQDZdzkwr5FWTX4%3D&reserved=0