Re: [PATCH net] nfc: s3fwrn5: validate the firmware image size against the firmware file

[netdev-bot+sashiko@kernel.org]

Thank you for your contribution! Sashiko AI review found 1 potential
issue(s) to consider.

Critical: 0 · High: 1 · Medium: 0 · Low: 0 (1)

If you believe none of the issues are legitimate, please reply to each
issue of Medium severity or higher and explain why the AI is wrong.
If any of them are legitimate and you plan to respin, please let patchwork
know by including "pw-bot: cr" as a separate line at the end of your reply
(one such reply per series is enough).

- [High] The fix is incomplete because the signature offsets and sizes in
  the firmware header are still not checked.

Pre-existing issues:
- [Low] struct s3fwrn5_fw_cmd_get_bootinfo_rsp is 12 bytes
  (hw_version[4], sector_size, page_size, frame_max_size,…

--

nfc: s3fwrn5: validate the firmware image size against the firmware file

This patch adds bounds checks to the s3fwrn5 firmware download path. It
checks the GET_BOOTINFO response length and rejects an image offset past
the end of the firmware file. It also checks that sector_size *
image_sectors, plus the 4 KiB sent per sector, fits in the firmware image
before the image is hashed and flashed.

> diff --git a/drivers/nfc/s3fwrn5/firmware.c b/drivers/nfc/s3fwrn5/firmware.c
> index 9145deec7f6c1..159afb8de7491 100644
> --- a/drivers/nfc/s3fwrn5/firmware.c
> +++ b/drivers/nfc/s3fwrn5/firmware.c
> @@ -93,7 +93,8 @@ static int s3fwrn5_fw_get_bootinfo(struct s3fwrn5_fw_info *fw_info,
>  		return ret;
>  
>  	hdr = (struct s3fwrn5_fw_header *) rsp->data;
> -	if (hdr->code != S3FWRN5_FW_RET_SUCCESS) {
> +	if (hdr->code != S3FWRN5_FW_RET_SUCCESS ||
> +	    rsp->len < S3FWRN5_FW_HDR_SIZE + 10) {

[Severity: Low]
This is a pre-existing issue, but struct s3fwrn5_fw_cmd_get_bootinfo_rsp
is 12 bytes:

struct s3fwrn5_fw_cmd_get_bootinfo_rsp {
	__u8 hw_version[4];
	__u16 sector_size;
	__u16 page_size;
	__u16 frame_max_size;
	__u16 hw_buffer_size;
};

The new length check and the existing memcpy() in
s3fwrn5_fw_get_bootinfo() both use the literal 10. That means
hw_buffer_size in the on-stack bootinfo in s3fwrn5_fw_setup() is never
filled in.

Nothing in drivers/nfc/s3fwrn5/ reads hw_buffer_size today, so this has
no functional effect.

Would it be cleaner to use sizeof(*bootinfo) here? Or a named constant
that documents a 10-byte wire format?

>  		ret = -EINVAL;
>  		goto out;
>  	}

[ ... ]

> @@ -307,6 +308,10 @@ int s3fwrn5_fw_request_firmware(struct s3fwrn5_fw_info *fw_info)
>  	memcpy(&fw->sig_size, fw->fw->data + 0x18, 4);
>  
>  	memcpy(&image_off, fw->fw->data + 0x1C, 4);
> +	if (image_off > fw->fw->size) {
> +		release_firmware(fw->fw);
> +		return -EINVAL;
> +	}
>  	fw->image = fw->fw->data + image_off;
>  	memcpy(&fw->image_sectors, fw->fw->data + 0x20, 4);
>  

[Severity: High]
image_off is now checked against fw->fw->size. The signature fields from
the same header are still used without any range check:

drivers/nfc/s3fwrn5/firmware.c:s3fwrn5_fw_request_firmware() {
	...
	memcpy(&sig_off, fw->fw->data + 0x14, 4);
	fw->sig = fw->fw->data + sig_off;
	memcpy(&fw->sig_size, fw->fw->data + 0x18, 4);
	...
	memcpy(&custom_sig_off, fw->fw->data + 0x24, 4);
	fw->custom_sig = fw->fw->data + custom_sig_off;
	memcpy(&fw->custom_sig_size, fw->fw->data + 0x28, 4);
	...
}

s3fwrn5_fw_setup() then picks one of the two pairs based on
s3fwrn5_fw_is_custom(&bootinfo). That depends on hw_version[2], which the
controller reports in GET_BOOTINFO.

The chosen pointer and size (truncated to u16) are then sent to the
controller:

s3fwrn5_fw_download()
  s3fwrn5_fw_enter_update_mode(fw_info, hash_data, SHA1_DIGEST_SIZE,
                               fw_info->sig, fw_info->sig_size)
    s3fwrn5_fw_prep_msg(fw_info, &msg, S3FWRN5_FW_MSG_DATA, 0,
                        sig_data, sig_size)
      skb_put_data(skb, data, len)

What happens if sig_off + sig_size, or custom_sig_off + custom_sig_size,
runs past fw->fw->size? Can skb_put_data() then read up to 64 KiB beyond
the firmware buffer?

Depending on how far the read goes, it would either oops on a guard page
or unmapped address, or copy nearby kernel memory into the DATA packet
sent to the controller.

The commit message says:

    Check that everything that is hashed and sent lies within the image,

The signature is sent in the same update sequence. Should both signature
extents also be checked against fw->fw->size here, for example with
check_add_overflow()?

-- 
Sashiko AI review · https://netdev-ai.bots.linux.dev/sashiko/#/patchset/179332800288.52644.4206885352837613064%40gmail.com