* Re: [PATCH] Input: iqs5xx: validate firmware record destination span
2026-07-15 8:38 [PATCH] Input: iqs5xx: validate firmware record destination span Pengpeng Hou
@ 2026-07-15 8:51 ` sashiko-bot
0 siblings, 0 replies; 2+ messages in thread
From: sashiko-bot @ 2026-07-15 8:51 UTC (permalink / raw)
To: Pengpeng Hou; +Cc: linux-input, dmitry.torokhov
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
^ permalink raw reply [flat|nested] 2+ messages in thread