Re: [PATCH] Input: atmel_mxt_ts - validate firmware frame bounds
From: sashiko-bot@kernel.org
Date: 2026-07-06 09:25:05
Thank you for your contribution! Sashiko AI review found 2 potential issue(s) to consider: Pre-existing issues: - [High] This problem wasn't introduced by this patch, but missing serialization in the sysfs `update_fw` attribute leads to double-free and use-after-free vulnerabilities. - [High] This problem wasn't introduced by this patch, but direct use of a vmapped firmware buffer for I2C transfers causes DMA mapping failures or memory corruption. -- commit 4f9a71983f301889709f29b76263c84f5ab07dfc Author: Pengpeng [off-list ref] Input: atmel_mxt_ts - validate firmware frame bounds This commit adds bounds checking to the firmware update loop in mxt_flash_fw() to ensure that the declared frame size and its CRC bytes fit within the remaining firmware image. This prevents reading past the end of the firmware buffer.
quoted hunk ↗ jump to hunk
diff --git a/drivers/input/touchscreen/atmel_mxt_ts.c b/drivers/input/touchscreen/atmel_mxt_ts.c index 2c0e5a67135b6..2aa52b4e5ff50 100644 --- a/drivers/input/touchscreen/atmel_mxt_ts.c +++ b/drivers/input/touchscreen/atmel_mxt_ts.c
[ ... ]
quoted hunk ↗ jump to hunk
@@ -2927,10 +2927,19 @@ static int mxt_flash_fw(struct mxt_data *data, const struct firmware *fw) if (error) return error; + if (fw->size - pos < 2) { + dev_err(dev, "Truncated frame size in firmware\n"); + return -EINVAL; + } + frame_size = ((*(fw->data + pos) << 8) | *(fw->data + pos + 1)); /* Take account of CRC bytes */ frame_size += 2; + if (frame_size > fw->size - pos) { + dev_err(dev, "Truncated frame in firmware\n"); + return -EINVAL; + } /* Write one frame to device */ error = mxt_bootloader_write(data, fw->data + pos, frame_size);
[Severity: High]
This isn't a bug introduced by this patch, but does this direct use of the
firmware buffer for I2C transfers risk DMA mapping failures?
The request_firmware() function can map the firmware buffer using vmap()
for files larger than PAGE_SIZE.
When mxt_bootloader_write() passes this potentially vmapped buffer directly
into i2c_transfer(), an underlying I2C adapter driver using DMA will
attempt to map a vmapped address, which yields an invalid physical address.
Should we bounce this buffer into memory allocated with kmalloc() before
passing it to the I2C subsystem?
[Severity: High]
This isn't a bug introduced by this patch, but does the firmware update path
lack serialization?
Concurrent writes to the sysfs update_fw attribute can execute mxt_load_fw()
in parallel:
mxt_update_fw_store() {
...
error = mxt_load_fw(dev, MXT_FW_NAME);
}
If both threads evaluate the bootloader state as true simultaneously,
they will both execute the cleanup routines:
mxt_load_fw() {
if (!data->in_bootloader) {
mxt_free_input_device(data);
mxt_free_object_table(data);
...
}
Could this result in a double-free on pointers like data->raw_info_block
and a double-unregister of the input device?
if (error) return error;
-- Sashiko AI review · https://sashiko.dev/#/patchset/20260706091144.76132-1-pengpeng@iscas.ac.cn?part=1