Re: [PATCH 09/13] HID: ft260: gpio: group sysfs attrs per HID interface

[sashiko-bot@kernel.org]

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

New issues:
- [Medium] The `i2c_enable` sysfs attribute is unintentionally removed for dual-mode devices.
- [Medium] The `i2c_reset` sysfs attribute is incorrectly exposed on the UART interface group.

Pre-existing issues:
- [High] Premature free of `dev` in `ft260_remove()` leads to multiple Use-After-Free conditions.
- [High] Double free and Use-After-Free of the `dev` structure in the probe error paths.
- [High] Missing and incorrectly ordered teardown of timer and workqueue leads to Use-After-Free.
- [High] On device hot-unplug, the driver frees critical resources before the TTY port is completely hung up, allowing concurrent TTY operations to trigger Use-After-Free.
- [High] Race condition Use-After-Free due to manual `kfree(dev)` before `devres` cleans up the registered gpiochip.
--

--- Patch [9]: [PATCH 09/13] HID: ft260: gpio: group sysfs attrs per HID interface ---
commit c899807eb1ab1f737fe08c45424f6b6a6de53615
Author: Michael Zaidman <michael.zaidman@gmail.com>

HID: ft260: gpio: group sysfs attrs per HID interface

Split the single sysfs attribute group into per-interface groups based
on chip mode (DCNF0/DCNF1).

> diff --git a/drivers/hid/hid-ft260.c b/drivers/hid/hid-ft260.c
> index 554d8e3ddb6db..0a1875a1a7eca 100644
> --- a/drivers/hid/hid-ft260.c
> +++ b/drivers/hid/hid-ft260.c

[ ... ]

> @@ -1603,11 +1603,35 @@ static const struct attribute_group ft260_attr_group = {
>  		  &dev_attr_gpio2_func.attr,
>  		  &dev_attr_gpioa_func.attr,
>  		  &dev_attr_gpiog_func.attr,
> +		  &dev_attr_clock_ctl.attr,
> +		  &dev_attr_i2c_reset.attr,
> +		  &dev_attr_clock.attr,
> +		  NULL
> +	}
> +};
> +
> +static const struct attribute_group ft260_i2c_chip_mode_0_3_attr_group = {
> +	.attrs = (struct attribute *[]) {
> +		  &dev_attr_hid_over_i2c_en.attr,
> +		  &dev_attr_i2c_reset.attr,
> +		  &dev_attr_clock.attr,
> +		  NULL
> +	}
> +};

[Severity: Medium]
Is the omission of the dev_attr_i2c_enable.attr intentional for dual-mode
devices?

The commit message mentions splitting the groups per interface based on chip
mode, but it looks like the i2c_enable attribute was completely dropped from
the dual-mode I2C group here. 

Could this silently break userspace applications relying on this sysfs entry?

> +
> +static const struct attribute_group ft260_uart_chip_mode_0_2_3_attr_group = {
> +	.attrs = (struct attribute *[]) {
> +		  &dev_attr_chip_mode.attr,
> +		  &dev_attr_pwren_status.attr,
> +		  &dev_attr_suspend_status.attr,
> +		  &dev_attr_power_saving_en.attr,
> +		  &dev_attr_gpio2_func.attr,
> +		  &dev_attr_gpioa_func.attr,
> +		  &dev_attr_gpiog_func.attr,
>  		  &dev_attr_uart_mode.attr,
>  		  &dev_attr_uart_dcd_ri.attr,
>  		  &dev_attr_clock_ctl.attr,
>  		  &dev_attr_i2c_reset.attr,

[Severity: Medium]
Does this unintentionally expose the I2C reset attribute on the UART
interface?

According to the patch description, the UART interface should only contain
GPIO and UART attributes, but dev_attr_i2c_reset.attr is included here.

> -		  &dev_attr_clock.attr,
>  		  NULL
>  	}
>  };

[ ... ]

> @@ -2306,12 +2339,14 @@ static int ft260_uart_probe(struct ft260_device *dev,
>  	cfg->uart_mode = FT260_UART_CFG_FLOW_CTRL_NONE;
>  
>  	if (dev->chip_mode & FT260_MODE_UART ||
> -	    dev->chip_mode == FT260_MODE_ALL) {
> +	    dev->chip_mode == FT260_MODE_ALL ||
> +	    dev->chip_mode == FT260_MODE_BOTH) {
>  		ret = ft260_gpio_init(dev, cfg);
>  		if (ret)
>  			goto err_hid_report;
>  
> -		ret = sysfs_create_group(&hdev->dev.kobj, &ft260_attr_group);
> +		ret = sysfs_create_group(&hdev->dev.kobj,
> +					 &ft260_uart_chip_mode_0_2_3_attr_group);
>  		if (ret < 0) {
>  			hid_err(hdev, "failed to create sysfs attrs\n");
>  			goto err_hid_report;

[Severity: High]
This is a pre-existing issue, but can jumping to err_hid_report lead to a
double free or use-after-free of the dev structure?

If sysfs_create_group() fails, the error path goes to err_hid_report, which
calls ft260_uart_port_remove(dev). This drops the TTY port reference via
tty_port_put().

If the TTY is not open, the refcount drops to 0, and the port destructor
frees dev immediately. The function then returns the error code back to
ft260_probe(), which unconditionally jumps to hid_fail and executes
kfree(dev).

Also, does this error path bypass workqueue cancellation?

The error path here deletes the timer but appears to completely omit calling
cancel_work_sync(), which could trigger a use-after-free if the timer had
already scheduled the work before dev is freed.

Finally, does the manual kfree(dev) in ft260_probe() race with devres?

The ft260_gpio_init() function registers a gpiochip using
devm_gpiochip_add_data(). The devres cleanup won't run until after the probe
function fully returns, leaving a window where the active gpiochip can be
accessed by userspace while its private data (dev) has already been freed.

>  		}

[ ... ]

> @@ -2418,12 +2453,19 @@ static void ft260_remove(struct hid_device *hdev)
>  		ft260_uart_port_remove(dev);
>  		/* dev is still needed, so we will free it in _destroy func */
>  		if (dev->chip_mode & FT260_MODE_UART ||
> -		    dev->chip_mode == FT260_MODE_ALL)
> -			sysfs_remove_group(&hdev->dev.kobj, &ft260_attr_group);
> +		    dev->chip_mode == FT260_MODE_ALL ||
> +		    dev->chip_mode == FT260_MODE_BOTH)
> +			sysfs_remove_group(&hdev->dev.kobj,
> +					   &ft260_uart_chip_mode_0_2_3_attr_group);

[Severity: High]
This is a pre-existing issue, but does checking dev->chip_mode here risk
dereferencing a freed pointer?

When ft260_uart_port_remove(dev) is called just above this block, it drops
the TTY port reference. If the port isn't open, the port destructor runs
and immediately frees dev. Accessing dev->chip_mode and removing the sysfs
group afterward would then be a use-after-free.

Does the order of operations in ft260_remove() also leave the device
vulnerable to incoming USB events after it is freed?

Because hid_hw_stop() is called much later in this function, there is a
window where incoming HID reports can trigger ft260_raw_event() on a
freed dev.

Are there concurrent teardown risks during device hot-unplug?

Looking at ft260_uart_port_remove(), kfifo_free() is called before the
asynchronous tty_port_tty_hangup(). If userspace writes to the TTY during
this window, ft260_uart_write() could push data into the explicitly
freed xmit_fifo.

Could the workqueue also be incorrectly rescheduled during unbind?

Earlier in ft260_remove(), cancel_work_sync(&dev->wakeup_work) is called
before timer_delete_sync() (which happens inside ft260_uart_port_remove).
This allows the timer to expire after the work is canceled, rescheduling
the work to execute after dev is freed.

>  
>  	} else {
>  		if (dev->chip_mode == FT260_MODE_I2C)

-- 
Sashiko AI review · https://sashiko.dev/#/patchset/20260822213941.98882-1-michael.zaidman@gmail.com?part=9