Re: [PATCH] HID: ft260: fix SMBus block read protocol handling

[sashiko-bot@kernel.org]

Thank you for your contribution! Sashiko AI review found 1 potential issue(s) to consider:

Pre-existing issues:
- [Critical] A Time-of-Check to Time-of-Use (TOCTOU) race condition in the I2C read timeout handling allows a malicious USB device to execute a Use-After-Free write on the kernel stack. This problem wasn't introduced by this patch, but it is a critical pre-existing flaw. `ft260_i2c_read` sets `dev->read_buf` to point to a stack variable and waits for completion. If the device delays the response causing a timeout, the function clears `dev->read_buf` to NULL and returns, popping the stack frame. Concurrently, `ft260_raw_event` checks `dev->read_buf != NULL` without locking and then performs a `memcpy`. A properly timed malicious device response can pass the NULL check before the timeout clears it, resulting in the `memcpy` writing device-controlled payload data into freed kernel stack memory, potentially hijacking control flow.
--

commit b044839cc37c642e14b8d9de071b491bae721de9
Author: Raman Varabets <kernel-linux-20260610-80b7ab08@raman.v1.sg>

HID: ft260: fix SMBus block read protocol handling

For I2C_SMBUS_BLOCK_DATA reads, ft260_smbus_xfer() passed
data->block[0] + 1 as the read length. But on a block read the byte
count is supplied by the slave as the first byte of the response;
data->block[0] is not initialized by the caller, so the transfer
length was taken from stale buffer contents, and the count byte the
slave did return was stored without any validation.

> diff --git a/drivers/hid/hid-ft260.c b/drivers/hid/hid-ft260.c
> index 70e2eedb465af..946ed0c6fc39f 100644
> --- a/drivers/hid/hid-ft260.c
> +++ b/drivers/hid/hid-ft260.c
> @@ -502,15 +502,23 @@ static int ft260_i2c_read(struct ft260_device *dev, u8 addr, u8 *data,
>  	struct ft260_i2c_read_request_report rep;
>  	struct hid_device *hdev = dev->hdev;
>  	u8 bus_busy = 0;
> +	/*
> +	 * STOP terminates the last chunk; clear means hold the bus so a
> +	 * follow-up call continues the same I2C transaction.
> +	 */
> +	bool want_stop = !!(flag & FT260_FLAG_STOP);

[Severity: Critical]
This is a pre-existing issue, but is there a TOCTOU race condition in the I2C
read timeout handling that allows a malicious USB device to execute a
use-after-free write on the kernel stack?

Looking at ft260_i2c_read(), it sets dev->read_buf to point to the provided
data pointer and waits for completion. If the device delays the response
causing a timeout, the function clears dev->read_buf to NULL and returns,
popping the stack frame:

drivers/hid/hid-ft260.c:ft260_i2c_read() {
        ...
		if (!wait_for_completion_timeout(&dev->wait, timeout)) {
			ret = -ETIMEDOUT;
			ft260_i2c_reset(hdev);
			goto ft260_i2c_read_exit;
		}
        ...
ft260_i2c_read_exit:
	dev->read_buf = NULL;
	return ret;
}

Concurrently, ft260_raw_event() checks dev->read_buf != NULL without taking
any locks and then performs a memcpy():

drivers/hid/hid-ft260.c:ft260_raw_event() {
        ...
		if ((dev->read_buf == NULL) ||
		    (xfer->length > dev->read_len - dev->read_idx)) {
        ...
		}

		memcpy(&dev->read_buf[dev->read_idx], &xfer->data,
		       xfer->length);
        ...
}

Could a properly timed malicious device response pass the NULL check before the
timeout clears it, resulting in the memcpy() writing device-controlled payload
data into freed kernel stack memory?

-- 
Sashiko AI review · https://sashiko.dev/#/patchset/20260610134119.3313484-1-kernel-linux-20260610-80b7ab08@raman.v1.sg?part=1