[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.