Re: [PATCH 13/13] HID: ft260: i2c: abort in-flight transfers with STOP before reset

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

New issues:
- [High] Use-After-Free in ft260_remove() due to premature kfree(dev) before stopping hardware.
- [High] Flawed cleanup order in UART device removal leads to Use-After-Free.

Pre-existing issues:
- [High] Divide-by-zero panic in `ft260_hid_output_report_check_status` if the device provides a clock frequency of 0.
--

--- Patch [13]: [PATCH 13/13] HID: ft260: i2c: abort in-flight transfers with STOP before reset ---
commit c3cb7a3558ab0556a46bf87d920daa2c07f916c4
Author: Michael Zaidman <[email protected]>

HID: ft260: i2c: abort in-flight transfers with STOP before reset

This commit introduces ft260_i2c_abort() to issue a STOP-only write before
resetting the I2C controller when a transaction needs to be torn down. This
prevents the controller from being stuck in a BUS_BUSY state which can block
subsequent START conditions. It updates various I2C and SMBus read/write
status checks to use this new abort mechanism instead of a simple reset.

> diff --git a/drivers/hid/hid-ft260.c b/drivers/hid/hid-ft260.c
> index 9ae688f6208f2..a35a1af2d7e57 100644
> --- a/drivers/hid/hid-ft260.c
> +++ b/drivers/hid/hid-ft260.c
[ ... ]
> @@ -538,6 +538,33 @@ static int ft260_i2c_reset(struct hid_device *hdev)
[ ... ]
> +static int ft260_i2c_abort(struct hid_device *hdev, u8 addr)
> +{
> +	ft260_i2c_stop(hdev, addr);
> +	return ft260_i2c_reset(hdev);
> +}
> +
>  static int ft260_xfer_status(struct ft260_device *dev, u8 bus_busy)
>  {
>  	struct hid_device *hdev = dev->hdev;

[Severity: High]
This is a pre-existing issue, but can this lead to a divide-by-zero panic if
a misbehaving USB device provides a clock value of 0?

In ft260_xfer_status(), the device-provided clock value is trusted without
bounds checking:

    dev->clock = le16_to_cpu(report.clock);

Then in ft260_hid_output_report_check_status(), an unchecked division by
dev->clock occurs:

    usec = len * 9000 / dev->clock;

[ ... ]

While reviewing this patch, I also noticed a couple of other issues in code
not directly modified by this commit:

[Severity: High]
Can an incoming HID report access the dev structure after it has been freed?

In ft260_remove(), dev is explicitly freed:

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

But if hid_hw_stop() is called after this, an incoming report could trigger
ft260_raw_event(), which dereferences the freed dev:

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

[Severity: High]
Does this cleanup order leave a window where the work could be re-queued and
execute after the port is freed?

In ft260_remove(), the work is cancelled:

drivers/hid/hid-ft260.c:ft260_remove() {
    ...
    cancel_work_sync(&dev->wakeup_work);
    ...
}

However, the timer that queues this work isn't deleted until later in
ft260_uart_port_remove():

drivers/hid/hid-ft260.c:ft260_uart_port_remove() {
    ...
    timer_delete_sync(&port->wakeup_timer);
    ...
}

If the timer fires in this window, it can re-queue the work. Later, the
structure is freed:

drivers/hid/hid-ft260.c:ft260_uart_port_destroy() {
    ...
    kfree(port);
    ...
}

If the asynchronously queued work executes after the port is freed, could it
cause a use-after-free in ft260_uart_do_wakeup()?

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