Re: [PATCH 04/13] HID: ft260: i2c: reduce driver module loading time

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

New issues:
- [High] The dynamic I2C read timeout incorrectly applies a 25ms timeout for last-chunk reads up to 180 bytes, causing spurious timeouts for chunks > 60 bytes.
- [High] Delayed USB HID reports from prematurely timed-out I2C transactions corrupt subsequent I2C transfers.

Pre-existing issues:
- [Critical] Double free of `dev` on UART probe error path.
- [High] The driver frees its private data before stopping hardware I/O, allowing asynchronous incoming HID reports to cause a use-after-free.
- [High] Race condition in teardown sequence of wakeup timer and workqueue leads to Use-After-Free on device unplug.
- [High] A malicious USB device can cause a divide-by-zero kernel panic by reporting a 0 Hz clock.
--

--- Patch [4]: [PATCH 04/13] HID: ft260: i2c: reduce driver module loading time ---
commit ce4834500e105e8a3ed2770b588049de28ffe3cf
Author: Michael Zaidman <[email protected]>

HID: ft260: i2c: reduce driver module loading time

The commit adjusts the I2C read timeout to depend on the amount of data
being read. This is intended to reduce the module loading time significantly,
lowering the lm75 address range scan time to about 200ms.

> diff --git a/drivers/hid/hid-ft260.c b/drivers/hid/hid-ft260.c
> index cc3b8f5791dea..4435a39fce239 100644
> --- a/drivers/hid/hid-ft260.c
> +++ b/drivers/hid/hid-ft260.c
[ ... ]
> @@ -732,10 +739,12 @@ static int ft260_i2c_read(struct ft260_device *dev, u8 addr, u8 *data,
>  		flag = 0;	/* no fresh START - continue current transaction */
>  	do {
>  		if (len <= rd_data_max) {
> +			timeout = FT260_RD_ONE_REPORT_TO;

[Severity: High]
Does this logic aggressively override the timeout to 25ms for the final
chunk of any transfer, even if that chunk is 180 bytes long?

If an I2C read request is made where the final chunk is between 61 and 180
bytes (e.g., a 240-byte read), the condition if (len <= rd_data_max)
correctly matches the final chunk, but erroneously assigns it the
FT260_RD_ONE_REPORT_TO (25ms) timeout instead of the required 75ms.

As noted in the commit message, a 180-byte chunk physically requires about
27ms of wire time at 60kHz. A 25ms timeout is guaranteed to expire before the
hardware finishes, which could lead to a deterministic failure (-ETIMEDOUT)
on any I2C read larger than 60 bytes.

>  			rd_len = len;
>  			if (want_stop)
>  				flag |= FT260_FLAG_STOP;
>  		} else {
> +			timeout = FT260_RD_MULTI_REPORT_TO;
>  			rd_len = rd_data_max;
>  		}
>  		rd_data_max = FT260_RD_DATA_MAX;
[ ... ]
> @@ -762,8 +771,8 @@ static int ft260_i2c_read(struct ft260_device *dev, u8 addr, u8 *data,
>  			goto ft260_i2c_read_exit;
>  		}
>  
> -		timeout = msecs_to_jiffies(5000);
> -		if (!wait_for_completion_timeout(&dev->wait, timeout)) {
> +		timeout_jiffies = msecs_to_jiffies(timeout);
> +		if (!wait_for_completion_timeout(&dev->wait, timeout_jiffies)) {

[Severity: High]
Will the drastic reduction of the timeout bounds without a concurrent
mechanism to purge the HID input queue or use sequence numbers expose the
driver to data corruption?

If a premature timeout occurs in ft260_i2c_read() due to the reduced bounds,
it causes the function to clear dev->read_buf and return. However, the USB
host controller may still receive the delayed response.

When a subsequent I2C read starts, it repopulates dev->read_buf. Since
ft260_raw_event() lacks sequence numbers, could it blindly copy the stale
delayed USB report into the new buffer and call complete(), allowing the new
transaction to complete with invalid data?

>  			ret = -ETIMEDOUT;
>  			ft260_i2c_reset(hdev);
>  			goto ft260_i2c_read_exit;

[Severity: High]
This isn't a bug introduced by this patch, but does the driver free its
private data before stopping hardware I/O, allowing asynchronous incoming HID
reports to cause a use-after-free?

In ft260_remove():

	} else {
		// ...
		kfree(dev);
	}

	hid_hw_close(hdev);
	hid_hw_stop(hdev);

During device unplug, the driver unconditionally frees its private dev
structure before calling hid_hw_stop(hdev). Because the hardware and the USB
interrupt IN endpoint remain active until hid_hw_stop() is called, an
incoming HID report during this window will be dispatched to
ft260_raw_event(), which will attempt to acquire dev->read_lock on the
already freed dev memory.

[Severity: Critical]
This isn't a bug introduced by this patch, but is there a double free of dev
on the UART probe error path?

In ft260_probe():

hid_fail:
	kfree(dev);
alloc_fail:

If ft260_uart_probe() fails, it jumps to the error path and calls
ft260_uart_port_remove(dev). This triggers tty_port_put(), which drops the
final reference and calls ft260_uart_port_destroy(), explicitly freeing dev
via kfree(port).

The function then returns a failure code to ft260_probe(), which follows its
own error path to hid_fail where it executes kfree(dev) a second time on the
same pointer.

[Severity: High]
This isn't a bug introduced by this patch, but is there a race condition in
the teardown sequence of the wakeup timer and workqueue that leads to a
use-after-free on device unplug?

In ft260_remove():

	cancel_work_sync(&dev->wakeup_work);
	tty_port_unregister_device(&dev->port, ft260_tty_driver, dev->index);
	ft260_uart_port_remove(dev);

The cancel_work_sync() is executed before timer_delete_sync() (called within
ft260_uart_port_remove()), which fails to guarantee the workqueue will remain
empty.

Because the timer can fire between these two calls, ft260_uart_start_wakeup()
can erroneously call schedule_work() to re-enqueue the work. Once unbind
completes and dev is freed via the TTY destructor, the pending work executes
and accesses freed memory.

[Severity: High]
This isn't a bug introduced by this patch, but can a malicious USB device
cause a divide-by-zero kernel panic by reporting a 0 Hz clock?

In ft260_hid_output_report_check_status():

	/* transfer time = 1 / clock(KHz) * 9 bits * bytes */
	usec = len * 9000 / dev->clock;

The unvalidated clock value read from the hardware is used directly as a
divisor. If a malicious or malfunctioning USB device responds to the
FT260_I2C_STATUS feature request in ft260_xfer_status() with a report.clock
value of 0, the driver saves this to dev->clock without validation.
Subsequent calls will divide by dev->clock to calculate delay timings,
immediately crashing the kernel.

-- 
Sashiko AI review · https://sashiko.dev/#/patchset/[email protected]?part=4