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

[sashiko-bot@kernel.org]

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