[PATCH] Input: atmel_mxt_ts - validate firmware frame bounds

[PATCH] Input: atmel_mxt_ts - validate firmware frame bounds

[Pengpeng Hou]

From: Pengpeng <pengpeng@iscas.ac.cn>

mxt_flash_fw() walks a length-prefixed firmware image. The loop only
checks that the cursor is below fw->size before reading the two-byte
frame length and passing the declared frame to the bootloader write path.

Check that the length field and the complete frame, including CRC bytes,
fit in the remaining firmware image before reading or writing the frame.

Signed-off-by: Pengpeng <pengpeng@iscas.ac.cn>
---
 drivers/input/touchscreen/atmel_mxt_ts.c | 9 +++++++++
 1 file changed, 9 insertions(+)

diff --git a/drivers/input/touchscreen/atmel_mxt_ts.c b/drivers/input/touchscreen/atmel_mxt_ts.c
index 2c0e5a67135b..2aa52b4e5ff5 100644
--- a/drivers/input/touchscreen/atmel_mxt_ts.c
+++ b/drivers/input/touchscreen/atmel_mxt_ts.c
@@ -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);
-- 
2.43.0

Re: [PATCH] Input: atmel_mxt_ts - validate firmware frame bounds

[sashiko-bot]

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 <pengpeng@iscas.ac.cn>

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.

> 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
[ ... ]
> @@ -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