Re: [PATCH 06/13] HID: ft260: i2c: reduce bus-error message severity

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

New issues:
- [Low] Redundant `__func__` prefix in `ft260_dbg` logging.

Pre-existing issues:
- [High] Hardware is stopped after freeing device resources in `ft260_remove`, causing a Use-After-Free if a report arrives.
- [High] Accessing `dev` after it has been synchronously freed by `ft260_uart_port_remove`.
- [High] Double free of `dev` in the `ft260_probe` error path for UART interfaces.
- [High] The devres-managed GPIO chip remains active after the driver manually frees its context.
- [High] Improper cleanup order of workqueue and timer leads to a leaked work item and UAF.
--

--- Patch [6]: [PATCH 06/13] HID: ft260: i2c: reduce bus-error message severity ---
commit 726d7795b8219b2b5c0f6063a18546f73c323838
Author: Michael Zaidman <[email protected]>

HID: ft260: i2c: reduce bus-error message severity

This commit reduces the severity of I2C bus error messages during bus scans by
replacing hid_err with ft260_dbg. It keeps returning -EIO but avoids spamming
dmesg with routine failures.

> diff --git a/drivers/hid/hid-ft260.c b/drivers/hid/hid-ft260.c
> index 8db896f164cd0..1ed3701eaaf75 100644
> --- a/drivers/hid/hid-ft260.c
> +++ b/drivers/hid/hid-ft260.c
[ ... ]
> @@ -669,7 +669,7 @@ static int ft260_i2c_write(struct ft260_device *dev, u8 addr, u8 *data,
>  		ret = ft260_hid_output_report_check_status(dev, (u8 *)rep,
>  							   wr_len + 4);
>  		if (ret < 0) {
> -			hid_err(hdev, "%s: failed with %d\n", __func__, ret);
> +			ft260_dbg("%s: failed with %d\n", __func__, ret);

[Severity: Low]
Does ft260_dbg already prepend the function name to the log output? If so,
keeping __func__ in the format string here might cause the function name to be
printed twice.

>  			return ret;
>  		}
>  
[ ... ]
> @@ -710,7 +710,7 @@ static int ft260_smbus_write(struct ft260_device *dev, u8 addr, u8 cmd,
>  
>  	ret = ft260_hid_output_report_check_status(dev, (u8 *)rep, len);
>  	if (ret < 0)
> -		hid_err(dev->hdev, "%s: failed with %d\n", __func__, ret);
> +		ft260_dbg("%s: failed with %d\n", __func__, ret);

[Severity: Low]
Similarly, does this result in a redundant function name in the log output?

>  
>  	return ret;
>  }

While reviewing this code, I also noticed a few other items in the driver.

[Severity: High]
This is a pre-existing issue, but is it possible for a use-after-free to occur
during device unbind in ft260_remove()?

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

Because hid_hw_stop() is called at the end of the function, the HID hardware
is still active and can process incoming reports. If a report arrives, it
triggers ft260_raw_event() which accesses the already freed dev:

drivers/hid/hid-ft260.c:ft260_raw_event() {
    struct ft260_device *dev = hid_get_drvdata(hdev);
    ...
}

Can this sequence be triggered by hot-unplugging or receiving interrupts
during removal?

[Severity: High]
This isn't a bug introduced by this patch, but does ft260_remove() access dev
after it has been synchronously freed?

drivers/hid/hid-ft260.c:ft260_remove() {
    ...
    ft260_uart_port_remove(dev);
    /* dev is still needed, so we will free it in _destroy func */
    if (dev->iface_id == 0)
    ...
}

If the port is not actively opened by userspace, ft260_uart_port_remove()
drops the tty_port refcount to 0, which invokes the destructor and frees dev.
Is it safe to access dev->iface_id immediately after this call?

[Severity: High]
This is a pre-existing issue, but could the error path in ft260_probe() lead
to a double free for UART interfaces?

drivers/hid/hid-ft260.c:ft260_uart_probe() {
    ...
err_register_tty:
    ft260_uart_port_remove(dev);
    return ret;
}

If ft260_uart_probe() fails and drops the port reference, dev is freed. The
failure propagates back to ft260_probe(), which then executes:

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

Does this cause kfree(dev) to be called on an already freed pointer?

[Severity: High]
This isn't a bug introduced by this patch, but does devres leave the GPIO chip
active after its private data is freed?

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

The driver registers a GPIO chip using devm_gpiochip_add_data() and assigns
dev as its private data. Since devres automatically cleans up the GPIO chip
only after the remove callback fully completes, can userspace race to invoke
GPIO operations during this window and dereference the freed dev?

[Severity: High]
This is a pre-existing issue, but does the cleanup order of the timer and
workqueue leak a work item and cause a use-after-free?

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

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

Since cancel_work_sync() is invoked before the timer is shut down, could the
timer fire concurrently and invoke schedule_work() again? This would leave a
pending work item that executes after the driver is unloaded and its private
data freed. Should timer_delete_sync() occur before cancel_work_sync()?

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