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

[Paul Menzel <pmenzel@molgen.mpg.de>]

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.ai).
> 
> Fixes: 689ca16e5232 ("Bluetooth: NXP: Add protocol support for NXP Bluetooth chipsets")
> Cc: stable@vger.kernel.org
> Assisted-by: 0sec:claude-opus-4-8
> Signed-off-by: Doruk Tan Ozturk <doruk@0sec.ai>
> ---
>   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