Re: [PATCH v2 05/15] HID: ft260: i2c: silence sysfs store big-numbers

[sashiko-bot@kernel.org]

Thank you for your contribution! Sashiko AI review found 1 potential issue(s) to consider:
- [High] HID: ft260: kfree() of dev before devres unregisters gpio_chip
--

commit 6921079604110581f3bdc063a11efc4ea28f578a
Author: Michael Zaidman <michael.zaidman@gmail.com>

HID: ft260: i2c: silence sysfs store big-numbers

This series adds UART and GPIO support to the FT260 HID driver, adjusts the
I2C read timeout, and updates FT260_ATTR_STORE to return the sysfs write byte
count and convert 16-bit attributes to little endian.

> diff --git a/drivers/hid/hid-ft260.c b/drivers/hid/hid-ft260.c
> index 64c9c940db46e..d2c5c9ed79276 100644
> --- a/drivers/hid/hid-ft260.c
> +++ b/drivers/hid/hid-ft260.c

[ ... ]

> @@ -1043,53 +2307,35 @@ static int ft260_probe(struct hid_device *hdev, const struct hid_device_id *id)
>  		 version.chip_code[0], version.chip_code[1],
>  		 version.chip_code[2], version.chip_code[3]);
>  
> -	ret = ft260_is_interface_enabled(hdev);
> -	if (ret <= 0)
> +	ret = ft260_get_interface_type(dev, &cfg);
> +	if (ret <= FT260_IFACE_NONE)
>  		goto err_hid_close;
>  
> -	hid_info(hdev, "USB HID v%x.%02x Device [%s] on %s\n",
> -		hdev->version >> 8, hdev->version & 0xff, hdev->name,
> -		hdev->phys);
> -
> -	hid_set_drvdata(hdev, dev);
> -	dev->hdev = hdev;
> -	dev->adap.owner = THIS_MODULE;
> -	dev->adap.class = I2C_CLASS_HWMON;
> -	dev->adap.algo = &ft260_i2c_algo;
> -	dev->adap.quirks = &ft260_i2c_quirks;
> -	dev->adap.dev.parent = &hdev->dev;
> -	snprintf(dev->adap.name, sizeof(dev->adap.name),
> -		 "FT260 usb-i2c bridge");
> -
> -	mutex_init(&dev->lock);
> -	spin_lock_init(&dev->read_lock);
> -	init_completion(&dev->wait);
> -
> -	ret = ft260_xfer_status(dev, FT260_I2C_STATUS_BUS_BUSY);
> -	if (ret)
> -		ft260_i2c_reset(hdev);
> -
> -	i2c_set_adapdata(&dev->adap, dev);
> -	ret = i2c_add_adapter(&dev->adap);
> -	if (ret) {
> -		hid_err(hdev, "failed to add i2c adapter\n");
> -		goto err_hid_close;
> -	}
> -
> -	ret = sysfs_create_group(&hdev->dev.kobj, &ft260_attr_group);
> -	if (ret < 0) {
> -		hid_err(hdev, "failed to create sysfs attrs\n");
> -		goto err_i2c_free;
> +	if (ret == FT260_IFACE_I2C) {
> +		ret = ft260_i2c_probe(dev, &cfg);
> +		if (ret)
> +			goto err_hid_close;
> +	} else {
> +		ret = ft260_uart_probe(dev, &cfg);
> +		if (ret)
> +			goto err_uart;
>  	}
>  
>  	return 0;
>  
> -err_i2c_free:
> -	i2c_del_adapter(&dev->adap);
> +err_uart:
> +	hid_hw_close(hdev);
> +	hid_hw_stop(hdev);
> +	tty_port_put(&dev->port);
> +	return ret;
> +
>  err_hid_close:
>  	hid_hw_close(hdev);
>  err_hid_stop:
>  	hid_hw_stop(hdev);
> +hid_fail:
> +	kfree(dev);
> +alloc_fail:
>  	return ret;
>  }
>  
> @@ -1100,25 +2346,47 @@ static void ft260_remove(struct hid_device *hdev)
>  	if (!dev)
>  		return;
>  
> -	sysfs_remove_group(&hdev->dev.kobj, &ft260_attr_group);
> -	i2c_del_adapter(&dev->adap);
> +	if (dev->iface_type == FT260_IFACE_UART) {
> +		timer_delete_sync(&dev->wakeup_timer);
> +		cancel_work_sync(&dev->wakeup_work);
> +		tty_port_unregister_device(&dev->port, ft260_tty_driver,
> +					   dev->index);
> +		ft260_uart_port_remove(dev);
> +		if (dev->iface_id == 0)
> +			sysfs_remove_group(&hdev->dev.kobj, &ft260_attr_group);
> +	} else {
> +		sysfs_remove_group(&hdev->dev.kobj, &ft260_attr_group);
> +		i2c_del_adapter(&dev->adap);
> +	}
>  
>  	hid_hw_close(hdev);
>  	hid_hw_stop(hdev);
> +
> +	if (dev->iface_type == FT260_IFACE_UART)
> +		ft260_uart_port_put(dev);
> +	else
> +		kfree(dev);
>  }

[Severity: High]
As reported by Sashiko on v1 [1], in drivers/hid/hid-ft260.c, ft260_probe()
allocates struct ft260_device *dev using kzalloc_obj(*dev, GFP_KERNEL), while
ft260_gpio_init() registers the GPIO chip with devres on hdev->dev:

drivers/hid/hid-ft260.c:ft260_gpio_init() {
	...
	ret = devm_gpiochip_add_data(&hdev->dev, dev->gc, dev);
	...
}

In the HID core (drivers/hid/hid-core.c), __hid_device_probe() and
hid_device_remove() only release the devres group via devres_release_group()
after hdrv->probe() or hdrv->remove() returns.

When ft260_remove() runs (or when sysfs_create_group() fails after
ft260_gpio_init() in ft260_i2c_probe() or ft260_uart_probe() and unwinds
through ft260_probe()), dev is freed via kfree(dev) or tty_port_put() /
ft260_uart_port_put() before the devm-managed gpio_chip is unregistered.

Can a concurrent GPIO operation (such as ft260_gpio_get(),
ft260_gpio_get_direction(), ft260_gpio_set(), or ft260_gpio_direction_set())
retrieve the freed dev pointer via gpiochip_get_data(gc) during this window
and cause a use-after-free?

[1] https://lore.kernel.org/r/20260822213941.98882-6-michael.zaidman@gmail.com

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