Re: [PATCH 11/13] HID: ft260: i2c: fix large write transaction failure

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

Pre-existing issues:
- [High] The `ft260_device` structure is freed before the HID hardware is stopped, creating a use-after-free vulnerability in the event handler.
- [High] A malicious USB device can cause a kernel panic via division by zero by reporting a clock frequency of 0, or by stalling the feature report read.
- [High] A malicious USB device can trigger an out-of-bounds heap read in `ft260_gpio_init` via an unvalidated `uart_mode` value.
--

--- Patch [11]: [PATCH 11/13] HID: ft260: i2c: fix large write transaction failure ---
commit 194ff3edbd401c08d64eb7640df73c9f274ff9ea
Author: Michael Zaidman <[email protected]>

HID: ft260: i2c: fix large write transaction failure

Fixes a regression where multi-report I2C writes failed because middle
reports were treated like a final STOP. It passes the bus-busy check
policy from the caller so intermediate chunks and the command phase of
combined transfers do not wait for idle.

Link: https://github.com/MichaelZaidman/hid-ft260/issues/35
Link: https://github.com/MichaelZaidman/hid-ft260/issues/42

> diff --git a/drivers/hid/hid-ft260.c b/drivers/hid/hid-ft260.c
> index a2f4b6e5a16bb..36687c086b408 100644
> --- a/drivers/hid/hid-ft260.c
> +++ b/drivers/hid/hid-ft260.c
> @@ -566,9 +566,14 @@ static int ft260_xfer_status(struct ft260_device *dev, u8 bus_busy)
>  	}
>  
>  	dev->clock = le16_to_cpu(report.clock);

[Severity: High]
This is a pre-existing issue, but can a malicious USB device cause a
kernel panic here via division by zero?

In ft260_xfer_status(), dev->clock is populated from the device's
FT260_I2C_STATUS feature report without validation against zero.

Later, during an I2C transfer in ft260_hid_output_report_check_status():

drivers/hid/hid-ft260.c:ft260_hid_output_report_check_status() {
    ...
    ret = ft260_hid_output_report(hdev, data, len);
    if (ret < 0) {
        ...
        return ret;
    }

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

A malicious device could return a clock frequency of 0, or stall the
initial feature report read (leaving dev->clock at its initialized 0).
When the first I2C payload is processed, does this cause a division by
zero?

> -	ft260_dbg("bus_status %#02x, clock %u\n", report.bus_status,
> -		  dev->clock);
> +	ft260_dbg("bus_status %#02x, clock %u, bus_busy %#02x\n",
> +		  report.bus_status, dev->clock, bus_busy);
>  

[ ... ]

[Severity: High]
This isn't a bug introduced by this patch, but is there a use-after-free
vulnerability in the event handler during device removal?

In ft260_remove(), the ft260_device structure is freed before the HID
hardware is stopped:

drivers/hid/hid-ft260.c:ft260_remove() {
    ...
        kfree(dev);
    }

    hid_hw_close(hdev);
    hid_hw_stop(hdev);
}

If an incoming hardware report is received between the memory being freed
and the hardware being stopped, the HID core will call ft260_raw_event().
This function retrieves the freed ft260_device and immediately dereferences
it:

drivers/hid/hid-ft260.c:ft260_raw_event() {
    ...
    spin_lock_irqsave(&dev->read_lock, irqflags);
    ...
}

Can an attacker time incoming reports to race with remove() and trigger a
use-after-free?

[Severity: High]
This is also a pre-existing issue, but does ft260_gpio_init() have an
out-of-bounds read when parsing the UART mode?

During device initialization, cfg->uart_mode is completely controlled by
the device. In ft260_gpio_init(), this value is used as an index into a
5-element array without bounds checking:

drivers/hid/hid-ft260.c:ft260_gpio_init() {
    ...
    if (mode & FT260_MODE_UART)
        dev->gpio_en |= dev->gpio_uart_mode[cfg->uart_mode];

    hid_info(hdev, "enabled GPIOs: %04x\n", dev->gpio_en);
    ...
}

Could a device returning a uart_mode value between 5 and 255 cause an
out-of-bounds read past the end of the ft260_device structure into
adjacent heap memory, and leak that 16-bit OOB value to the kernel log?

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