Re: [PATCH v2 03/15] HID: ft260: add GPIO support

[sashiko-bot@kernel.org]

Thank you for your contribution! Sashiko AI review found 1 potential issue(s) to consider:
- [High] HID: ft260: use-after-free from devm_gpiochip_add_data() outliving dev
--

commit 0ee92e34686aff73d8b504af1979dbbb02897cf0
Author: Michael Zaidman <michael.zaidman@gmail.com>

HID: ft260: add GPIO support

This commit adds gpiochip support for the FT260 multifunctional pins, exposes
pin function selection attributes via sysfs, and tracks enabled GPIO pins
based on the active I2C and UART configuration.

> diff --git a/drivers/hid/hid-ft260.c b/drivers/hid/hid-ft260.c
> index c859486cb28c8..e24f3151fdaf3 100644
> --- a/drivers/hid/hid-ft260.c
> +++ b/drivers/hid/hid-ft260.c

[ ... ]

> @@ -919,6 +1033,347 @@ static const struct i2c_algorithm ft260_i2c_algo = {

[ ... ]

> +static int ft260_gpio_init(struct ft260_device *dev,
> +			   struct ft260_get_system_status_report *cfg)
> +{

[ ... ]

> +	ret = devm_gpiochip_add_data(&hdev->dev, dev->gc, dev);
> +	if (ret < 0)
> +		hid_err(hdev, "cannot add GPIO chip %d\n", ret);
> +exit:
> +	return ret;
> +}

[Severity: High]
Can registering dev->gc with devm_gpiochip_add_data() in ft260_gpio_init() in
drivers/hid/hid-ft260.c cause a use-after-free on dev during device removal or
probe error rollback?

In drivers/hid/hid-core.c, hid_device_remove() calls hdrv->remove(hdev) before
releasing the device's devres group:

  hid_device_remove()
    ft260_remove()
      hid_hw_close(hdev);
      hid_hw_stop(hdev);
      if (dev->iface_type == FT260_IFACE_UART)
          ft260_uart_port_put(dev);
      else
          kfree(dev);
    devres_release_group(&hdev->dev, hdev->devres_group_id)
      devm_gpio_chip_release()
        gpiochip_remove(dev->gc)

Similarly, if sysfs_create_group() fails after ft260_gpio_init() succeeds in
ft260_i2c_probe() or ft260_uart_probe(), ft260_probe() closes and stops hdev
and frees dev (via kfree(dev) in err_hid_close, or via tty_port_put(&dev->port)
in err_uart) before __hid_device_probe() calls devres_release_group().

Until gpiochip_remove() runs, the GPIO chip remains live and registered with
dev as its private data pointer. If a gpiolib consumer or /dev/gpiochip
userspace ioctl invokes ft260_gpio_get(), ft260_gpio_set(),
ft260_gpio_direction_set(), or ft260_gpio_get_direction() during that window,
gpiochip_get_data(gc) will return the freed dev pointer, dereferencing freed
memory (such as dev->lock, dev->hdev, and dev->gpio_en) and issuing HID
transfers on a stopped hid_device.

Would it be safer to manage the gpio_chip registration explicitly with
gpiochip_add_data() and gpiochip_remove() before stopping hdev and freeing dev
in ft260_remove() and the probe error paths?

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