Re: [PATCH v2] mfd: iqs62x: reject zero-length firmware records

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

New issues:
- [Low] The commit message description prefix is not capitalized.

Pre-existing issues:
- [High] Using a scalar `u8` field as a flexible array triggers `CONFIG_FORTIFY_SOURCE` panics during `memcpy` due to a strict field-spanning read check.
--

--- Patch [1]: [PATCH v2] mfd: iqs62x: reject zero-length firmware records ---
commit 73503c1abf73dab697ccffa04ef0c4a85ba0c685
Author: Pengpeng Hou <[email protected]>

mfd: iqs62x: reject zero-length firmware records

This commit fixes a vulnerability in the iqs62x firmware parser where a 
zero-length record could cause the size cursor to move backwards. The patch 
adds a check to reject zero-length records and rewrites the remaining-size 
check to avoid an offset addition.

Link: https://lore.kernel.org/all/[email protected]/

> mfd: iqs62x: reject zero-length firmware records

[Severity: Low]
This isn't a bug, but the commit message description after the subsystem 
prefix is not capitalized. Should it be formatted as "mfd: iqs62x: Reject 
zero-length firmware records" to match the subsystem conventions?

> diff --git a/drivers/mfd/iqs62x.c b/drivers/mfd/iqs62x.c
> index ee017617d1d1b..412ae7777f729 100644
> --- a/drivers/mfd/iqs62x.c
> +++ b/drivers/mfd/iqs62x.c
> @@ -237,7 +237,7 @@ static int iqs62x_firmware_parse(struct iqs62x_core *iqs62x,
>  		fw_rec = (struct iqs62x_fw_rec *)(fw->data + pos);
>  		pos += sizeof(*fw_rec);
>  
> -		if (pos + fw_rec->len - 1 > fw->size) {
> +		if (!fw_rec->len || fw_rec->len - 1 > fw->size - pos) {
>  			ret = -EINVAL;
>  			break;
>  		}

[Severity: High]
This is a pre-existing issue, but does using a scalar u8 field as a variable
length array trigger a CONFIG_FORTIFY_SOURCE panic further down in this
function?

The data field is declared as a 1-byte scalar:

struct iqs62x_fw_rec {
	u8 type;
	u8 addr;
	u8 len;
	u8 data;
} __packed;

During parsing, a pointer to this 1-byte field is taken and passed as the 
source for memcpy:

drivers/mfd/iqs62x.c:iqs62x_firmware_parse() {
	...
	case IQS62X_FW_REC_TYPE_DATA:
		mask = 0;
		data = &fw_rec->data;
		len = fw_rec->len;
		break;
	...
	memcpy(fw_blk->data, data, len);
}

If the firmware contains an IQS62X_FW_REC_TYPE_DATA record with a length 
greater than 1, will memcpy abort and trigger a kernel panic due to the 
strict field-spanning read check?

-- 
Sashiko AI review · https://sashiko.dev/#/patchset/[email protected]?part=1