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 <[email protected]>
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/[email protected]?part=9
lmpx.com only provides a reader for public news (NNTP) servers. It is not
affiliated with the servers or forums shown here and is not responsible for
the content of articles, which is written by their respective authors.