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

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

[Raman Varabets]

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.

Implement the SMBus 2.0 block read protocol properly: read the count
byte first with a repeated START and no STOP, validate it against
I2C_SMBUS_BLOCK_MAX (resetting the bus and returning -EPROTO on a
bogus count), then read exactly that many data bytes and finish the
transaction with STOP. This keeps the whole sequence within a single
I2C transaction:

  S Addr+Wr A Reg A Sr Addr+Rd A Count A Data... P

To support issuing the two reads as one transaction, teach
ft260_i2c_read() to honor the caller's flags instead of always
forcing a START and unconditionally appending STOP to the last
chunk: START is only emitted if requested, and STOP is appended to
the final chunk only when the caller asked for it.

Signed-off-by: Raman Varabets <kernel-linux-20260610-80b7ab08@raman.v1.sg>
---
 drivers/hid/hid-ft260.c | 45 ++++++++++++++++++++++++++++++++++++-----
 1 file changed, 40 insertions(+), 5 deletions(-)

diff --git a/drivers/hid/hid-ft260.c b/drivers/hid/hid-ft260.c
index 70e2eedb4..946ed0c6f 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);
 
 	if ((flag & FT260_FLAG_START_REPEATED) == FT260_FLAG_START_REPEATED)
 		flag = FT260_FLAG_START_REPEATED;
-	else
+	else if (flag & FT260_FLAG_START)
 		flag = FT260_FLAG_START;
+	else
+		flag = 0;	/* no fresh START - continue current transaction */
 	do {
 		if (len <= rd_data_max) {
 			rd_len = len;
-			flag |= FT260_FLAG_STOP;
+			if (want_stop)
+				flag |= FT260_FLAG_STOP;
 		} else {
 			rd_len = rd_data_max;
 		}
@@ -708,14 +716,41 @@ static int ft260_smbus_xfer(struct i2c_adapter *adapter, u16 addr, u16 flags,
 		break;
 	case I2C_SMBUS_BLOCK_DATA:
 		if (read_write == I2C_SMBUS_READ) {
+			u8 count = 0;
+
+			/*
+			 * SMBus 2.0 section 6.5.7 block read in one I2C
+			 * transaction:
+			 *
+			 *   S Addr+Wr A Reg A Sr Addr+Rd A Count A Data... P
+			 *
+			 * The count is read separately and validated
+			 * before sizing the data read so a misbehaving
+			 * slave cannot drive a write past data->block[].
+			 */
 			ret = ft260_smbus_write(dev, addr, cmd, NULL, 0,
 						FT260_FLAG_START);
 			if (ret)
 				goto smbus_exit;
 
-			ret = ft260_i2c_read(dev, addr, data->block,
-					     data->block[0] + 1,
-					     FT260_FLAG_START_STOP_REPEATED);
+			ret = ft260_i2c_read(dev, addr, &count, 1,
+					     FT260_FLAG_START_REPEATED);
+			if (ret)
+				goto smbus_exit;
+
+			if (count == 0 || count > I2C_SMBUS_BLOCK_MAX) {
+				hid_warn(hdev,
+					 "smbus block read: invalid count %u from slave 0x%02x\n",
+					 count, addr);
+				ft260_i2c_reset(hdev);
+				ret = -EPROTO;
+				goto smbus_exit;
+			}
+
+			data->block[0] = count;
+
+			ret = ft260_i2c_read(dev, addr, data->block + 1,
+					     count, FT260_FLAG_STOP);
 		} else {
 			ret = ft260_smbus_write(dev, addr, cmd, data->block,
 						data->block[0] + 1,

base-commit: ab5fce87a778cb780a05984a2ca448f2b41aafbf
-- 
2.54.0

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

[sashiko-bot]

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