[PATCH] Input: iqs5xx: validate firmware record destination span

[PATCH] Input: iqs5xx: validate firmware record destination span

[Pengpeng Hou]

The firmware record parser checks that the record address starts within
the programmable map, but does not check that the complete record data
fits in that map. A record near the end of the map can therefore make
the copy to pmap exceed its destination span.

Check the record length against the remaining programmable map range
before copying the record data.

Fixes: 7b5bb55d0dad ("Input: add support for Azoteq IQS550/572/525")
Signed-off-by: Pengpeng Hou <pengpeng@iscas.ac.cn>
---
 drivers/input/touchscreen/iqs5xx.c | 3 ++-
 1 file changed, 2 insertions(+), 1 deletion(-)

diff --git a/drivers/input/touchscreen/iqs5xx.c b/drivers/input/touchscreen/iqs5xx.c
index c3cc37274335..88dcf72618df 100644
--- a/drivers/input/touchscreen/iqs5xx.c
+++ b/drivers/input/touchscreen/iqs5xx.c
@@ -785,7 +785,8 @@ static int iqs5xx_fw_file_parse(struct i2c_client *client,
 		switch (rec_type) {
 		case IQS5XX_REC_TYPE_DATA:
 			if (rec_addr < IQS5XX_CHKSM ||
-			    rec_addr > IQS5XX_PMAP_END) {
+			    rec_addr > IQS5XX_PMAP_END ||
+			    rec_len > IQS5XX_PMAP_END + 1 - rec_addr) {
 				dev_err(&client->dev,
 					"Invalid address at record %u\n",
 					rec_num);
-- 
2.43.0

Re: [PATCH] Input: iqs5xx: validate firmware record destination span

[sashiko-bot]

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

Pre-existing issues:
- [High] Concurrent writes to the `fw_file` sysfs attribute can lead to a double-registration of the input device, causing a kernel crash.
- [High] A checksum validation error in the firmware file is silently dropped, leading to incomplete firmware flashing.
- [Medium] The touchscreen input device may never be registered if a firmware update retry succeeds following an initial initialization failure.
--

--- Patch [1]: [PATCH] Input: iqs5xx: validate firmware record destination span ---
commit 897f4810526cdc25897398dbb6d1023c54aafc6a
Author: Pengpeng Hou <pengpeng@iscas.ac.cn>

Input: iqs5xx: validate firmware record destination span

This commit adds a length check in iqs5xx_fw_file_parse() to ensure that the
firmware record data fits within the remaining programmable map span. It
prevents out-of-bounds copies when a record is near the end of the map.

> diff --git a/drivers/input/touchscreen/iqs5xx.c b/drivers/input/touchscreen/iqs5xx.c
> index b9bbe8b3eab81..8559be7ed9a95 100644
> --- a/drivers/input/touchscreen/iqs5xx.c
> +++ b/drivers/input/touchscreen/iqs5xx.c
> @@ -786,7 +786,8 @@ static int iqs5xx_fw_file_parse(struct i2c_client *client,
>  		switch (rec_type) {
>  		case IQS5XX_REC_TYPE_DATA:
>  			if (rec_addr < IQS5XX_CHKSM ||
> -			    rec_addr > IQS5XX_PMAP_END) {
> +			    rec_addr > IQS5XX_PMAP_END ||
> +			    rec_len > IQS5XX_PMAP_END + 1 - rec_addr) {
>  				dev_err(&client->dev,
>  					"Invalid address at record %u\n",
>  					rec_num);

[Severity: High]
This isn't a bug introduced by this patch, but I noticed that later in this
same function, if a checksum validation failure occurs, the error code seems
to be swallowed:

drivers/input/touchscreen/iqs5xx.c:iqs5xx_fw_file_parse() {
...
		if (chksm != rec_chksm && rec_addr < IQS5XX_CSTM) {
			dev_err(&client->dev,
				"Incorrect checksum at record %u\n",
				rec_num);
			error = -EINVAL;
			break;
		}
...
	} while (rec_type != IQS5XX_REC_TYPE_EOF);

	return 0;
}

Since the break statement exits the do...while loop and the function
unconditionally returns 0 at the end, doesn't this result in the checksum
error being silently ignored?

Could this lead to an incomplete or corrupted memory map being flashed if
the firmware file is invalid?

[Severity: High]
This is a pre-existing issue, but I found a potential problem in
fw_file_store() where concurrent writes could lead to a double-registration
of the input device:

drivers/input/touchscreen/iqs5xx.c:fw_file_store() {
...
	bool input_reg = !iqs5xx->input;
...
	error = iqs5xx_fw_file_write(client, fw_file);
	if (error)
		return error;

	/*
	 * If the input device was not allocated already, it is guaranteed to
	 * be allocated by this point and can finally be registered.
	 */
	if (input_reg) {
		error = input_register_device(iqs5xx->input);
...
}

The input_reg flag is evaluated locklessly before calling
iqs5xx_fw_file_write(). If two threads concurrently write to fw_file
when the device is in bootloader mode (iqs5xx->input is NULL), both threads
would evaluate input_reg as true.

They would then serialize through iqs5xx_fw_file_write(), where the first
thread allocates the input device. When both threads return, wouldn't they
independently check their local input_reg variables and concurrently call
input_register_device(iqs5xx->input) on the exact same pointer?

[Severity: Medium]
This is another pre-existing issue, but there appears to be a separate
registration problem involving iqs5xx_axis_init() and fw_file_store():

drivers/input/touchscreen/iqs5xx.c:iqs5xx_axis_init() {
...
	if (!input) {
		input = devm_input_allocate_device(&client->dev);
		if (!input)
			return -ENOMEM;
...
		input_set_drvdata(input, iqs5xx);
		iqs5xx->input = input;
	}

	error = iqs5xx_read_word(client, IQS5XX_X_RES, &max_x);
	if (error)
		return error;
...
}

If a firmware update via fw_file_store() triggers iqs5xx_axis_init() and
allocates the input device, but a subsequent I2C read like iqs5xx_read_word()
fails, fw_file_store() will return an error without calling
input_register_device().

The iqs5xx->input pointer remains non-NULL. If user-space retries the
firmware update and it succeeds, won't fw_file_store() evaluate input_reg
as false and skip calling input_register_device() entirely?

-- 
Sashiko AI review · https://sashiko.dev/#/patchset/20260715083850.32155-1-pengpeng@iscas.ac.cn?part=1