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

[sashiko-bot@kernel.org]

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