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

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

[Joas Antonio dos Santos]

s3fwrn5_fw_download() hashes and flashes sector_size * image_sectors
bytes starting at fw->image.  sector_size comes from the GET_BOOTINFO
response of the controller's bootloader and image_sectors and the image
offset come from the firmware file, and none of them is checked against
the size of the file.  s3fwrn5_fw_update_sector() also always sends
16 * 256 bytes from the start of each sector.

The controller decides whether the update runs at all: it reports the
version that s3fwrn5_fw_check_version() compares against in
CORE_INIT_RSP.  So on every device up, as long as the firmware file is
installed, a controller that reports a
large sector_size makes sha1() and the update loop read far past the
end of the firmware buffer, and one that reports a small sector_size
gets up to 4 KiB past the end of the image sent to it in
UPDATE_SECTOR data packets.

Check that everything that is hashed and sent lies within the image,
reject a zero sector_size, reject an image offset past the end of the
file, and make sure the GET_BOOTINFO response is long enough before
copying the 10 bytes of boot information out of it.

Fixes: c04c674fadeb ("nfc: s3fwrn5: Add driver for Samsung S3FWRN5 NFC Chip")
Cc: stable@vger.kernel.org
Signed-off-by: Joas Antonio dos Santos <joasantonio108@gmail.com>
Assisted-by: Yaga Agent HackerSec Claude:claude-opus-5-5
---
Tested on arm64 QEMU with KASAN, with a test-only module that registers
s3fwrn5 through s3fwrn5_probe() on a virtual phy and a userspace
emulator for the NCI and bootloader protocols.  With a 4156 byte
firmware file (one 4096 byte sector) and the controller reporting an
older version:
- sector_size 0xffff: oops in sha1_ce_transform() from
  s3fwrn5_fw_download() without the patch, update refused with it;
- sector_size 16, 2 sectors: KASAN vmalloc-out-of-bounds in
  s3fwrn5_fw_prep_msg() while building an UPDATE_SECTOR data packet
  without the patch, update refused with it;
- sector_size 4096: the update runs the same with and without it.
Not tested on hardware.

 drivers/nfc/s3fwrn5/firmware.c | 24 ++++++++++++++++++++++--
 1 file changed, 22 insertions(+), 2 deletions(-)

diff --git a/drivers/nfc/s3fwrn5/firmware.c b/drivers/nfc/s3fwrn5/firmware.c
index 9145deec7..159afb8de 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) {
 		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);
 
@@ -409,11 +414,26 @@ int s3fwrn5_fw_download(struct s3fwrn5_fw_info *fw_info)
 {
 	struct device *dev = &fw_info->ndev->nfc_dev->dev;
 	struct s3fwrn5_fw_image *fw = &fw_info->fw;
+	size_t image_avail = fw->fw->data + fw->fw->size -
+			     (const u8 *)fw->image;
+	u64 total = (u64)fw_info->sector_size * fw->image_sectors;
 	u8 hash_data[SHA1_DIGEST_SIZE];
 	u32 image_size, off;
 	int ret;
 
-	image_size = fw_info->sector_size * fw->image_sectors;
+	/* sector_size comes from the bootloader and image_sectors from the
+	 * firmware file.  The whole image is hashed, and
+	 * s3fwrn5_fw_update_sector() sends 16 * 256 bytes from the start of
+	 * each sector, so all of that must lie within the firmware image.
+	 */
+	if (!fw_info->sector_size || total > image_avail ||
+	    (fw->image_sectors &&
+	     total - fw_info->sector_size + 16 * 256 > image_avail)) {
+		dev_err(dev, "Invalid firmware image size\n");
+		return -EINVAL;
+	}
+
+	image_size = total;
 
 	/* Compute SHA of firmware data */
 	sha1(fw->image, image_size, hash_data);
-- 
2.43.0

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

[netdev-bot+sinfo]

Hi!

This is an automated message. This series looks like a fix, but its
commit messages seem to be missing some information:

 - How the issue was discovered, e.g. hit in production, hit during
   development, syzbot report, manual code inspection, LLM or static
   analysis tool scan.

Please do not repost the series just to address the above. Instead,
reply to this email with the missing information, so that reviewers
can take it into account. If the series needs another revision for
other reasons, please include the information in the commit messages
then.

The evaluation is done by an LLM so it may be wrong, if you think
that is the case please reply and explain.

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

[Joas Antonio dos Santos]

This was found by LLM-assisted code review, as part of an audit of the
NFC drivers that are only reachable with real hardware (and so are not
exercised by syzbot).  It was then confirmed with KASAN on arm64 QEMU,
using a test-only module that registers s3fwrn5 through s3fwrn5_probe()
on a virtual phy and a userspace emulator of the controller's NCI and
bootloader protocols, as described below the --- of the patch.  It was
not hit in production or on hardware.

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

[netdev-bot+sashiko]

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