Re: [PATCH 01/13] HID: ft260: add serial driver

[sashiko-bot@kernel.org]

Thank you for your contribution! Sashiko AI review found 9 potential issue(s) to consider:
- [Critical] Use-After-Free of `hdev` and `xmit_fifo` if TTY operations occur after device removal.
- [High] Double free of the `dev` structure in the `ft260_probe()` error path.
- [High] Incorrect teardown ordering of the wakeup timer and workqueue creates a Use-After-Free race condition.
- [High] Device structure is freed before stopping the HID hardware, causing a Use-After-Free in the input report handler.
- [High] Opening an unregistered or disconnected FT260 TTY node crashes the kernel due to a missing NULL check.
- [Medium] Flawed TTY port index allocator logic generates duplicate indices, leading to sysfs collisions.
- [Medium] TTY data duplication due to incorrect return value in `ft260_uart_write()`.
- [Medium] Crashing dereference of `tty` in `ft260_uart_change_speed()` if the TTY is simultaneously closed.
- [Medium] Baud rate configuration is double-swapped on big-endian architectures, resulting in incorrect hardware configuration.
--

--- Patch [1]: [PATCH 01/13] HID: ft260: add serial driver ---
commit 2a24dbd449641441b22e899b0125b7e38412d4d3
Author: Christina Quast <contact@christina-quast.de>

HID: ft260: add serial driver

Add a TTY serial interface (/dev/ttyFTx) for FT260 USB HID devices,
providing baud rate configuration, data transfer, and termios settings
without requiring userspace HID report formatting.

Ported from: https://lore.kernel.org/all/20231218093153.192268-1-contact@christina-quast.de/
The follow-up v5 series stalled without being applied: https://lore.kernel.org/linux-input/20240216-ft260_review5-v5-1-36db44673ac7@christina-quast.de/

> diff --git a/drivers/hid/hid-ft260.c b/drivers/hid/hid-ft260.c
> index 64c9c940db46e..3faedfc5c4136 100644
> --- a/drivers/hid/hid-ft260.c
> +++ b/drivers/hid/hid-ft260.c
> @@ -1001,6 +1090,583 @@ static const struct attribute_group ft260_attr_group = {

[ ... ]

> +static int ft260_uart_add_port(struct ft260_device *port)
> +{
> +	int index = 0, ret = 0;
> +	struct ft260_device *dev;
> +
> +	spin_lock_init(&port->write_lock);
> +	if (kfifo_alloc(&port->xmit_fifo, FIFO_SIZE, GFP_KERNEL))
> +		return -ENOMEM;
> +
> +	mutex_lock(&ft260_uart_list_lock);
> +	list_for_each_entry(dev, &ft260_uart_device_list, device_list) {
> +		if (dev->index != index)
> +			break;
> +		index++;
> +	}
> +
> +	port->index = index;
> +	list_add(&port->device_list, &ft260_uart_device_list);
> +	mutex_unlock(&ft260_uart_list_lock);

[Severity: Medium]
Will this allocate duplicate indices when multiple devices are plugged in?

The iteration expects the list to be in ascending order to find a hole, but
list_add() prepends devices to the head, making the list descending. When
plugging in a third device, the loop will encounter index 1 first, mismatch
with index = 0, break early, and assign index 0 again, causing a sysfs
name collision.

[ ... ]

> +static void ft260_uart_port_remove(struct ft260_device *port)
> +{
> +	timer_delete_sync(&port->wakeup_timer);
> +
> +	mutex_lock(&ft260_uart_list_lock);
> +	list_del(&port->device_list);
> +	mutex_unlock(&ft260_uart_list_lock);
> +
> +	spin_lock(&port->write_lock);
> +	kfifo_free(&port->xmit_fifo);
> +	spin_unlock(&port->write_lock);
> +
> +	mutex_lock(&port->port.mutex);
> +	port->reschedule_work = false;
> +	tty_port_tty_hangup(&port->port, false);
> +	mutex_unlock(&port->port.mutex);
> +
> +	ft260_uart_port_put(port);
> +}

[Severity: Critical]
Can a use-after-free occur if userspace continues writing to the TTY after
device removal?

ft260_uart_port_remove() is called during teardown and synchronously frees
xmit_fifo. However, if userspace holds the TTY file open, the dev structure
remains alive. Subsequent writes to the TTY will call ft260_uart_write(),
which writes to the freed xmit_fifo and calls ft260_hid_output_report() using
the freed port->hdev.

[ ... ]

> +static int ft260_uart_open(struct tty_struct *tty, struct file *filp)
> +{
> +	int ret;
> +	struct ft260_device *port = tty->driver_data;
> +
> +	ret = tty_port_open(&port->port, tty, filp);

[Severity: High]
If port was NULL during ft260_uart_install(), won't this dereference NULL when
evaluating &port->port?

[ ... ]

> +static ssize_t ft260_uart_write(struct tty_struct *tty, const u8 *buf,
> +				 size_t count)
> +{
> +	struct ft260_device *port = tty->driver_data;
> +	struct hid_device *hdev = port->hdev;
> +	int len, ret;
> +
> +	len = kfifo_in_locked(&port->xmit_fifo, buf, count, &port->write_lock);
> +	ft260_dbg("count: %zu, len: %d", count, len);
> +
> +	ret = ft260_uart_transmit_chars(port);
> +	if (ret < 0) {
> +		hid_dbg(hdev, "Failed to transmit chars: %d\n", ret);
> +		return 0;
> +	}
> +
> +	ret = kfifo_len(&port->xmit_fifo);
> +	if (ret > 0) {
> +		hid_dbg(hdev, "Failed to  all kfifo data bytes\n");
> +		ft260_dbg("return: %d", len - ret);
> +		return len - ret;
> +	}
> +
> +	return len;
> +}

[Severity: Medium]
Does this logic silently duplicate data in the byte stream?

If kfifo_in_locked accepts all bytes, but ft260_uart_transmit_chars cannot
transmit them all immediately (leaving some in the FIFO), this function
returns len - kfifo_len(). The TTY layer assumes the difference was rejected
and will re-submit those bytes on the next call, appending them to the FIFO
again.

[ ... ]

> +static int ft260_uart_change_speed(struct ft260_device *port,
> +				   struct ktermios *termios,
> +				    struct ktermios *old)
> +{
> +	struct hid_device *hdev = port->hdev;
> +	unsigned int baud;
> +	struct ft260_configure_uart_request req;
> +	int ret;

[ ... ]

> +	baud = tty_termios_baud_rate(termios);
> +	if (baud == 0 || baud < FT260_CFG_BAUD_MIN || baud > FT260_CFG_BAUD_MAX) {
> +		struct tty_struct *tty = tty_port_tty_get(&port->port);
> +
> +		hid_err(hdev, "Invalid baud rate %d\n", baud);
> +		baud = 9600;
> +		tty_encode_baud_rate(tty, baud, baud);
> +		tty_kref_put(tty);
> +	}

[Severity: Medium]
Can tty_port_tty_get(&port->port) return NULL if the TTY is simultaneously
closed?

If it does, passing NULL to tty_encode_baud_rate() will unconditionally
dereference it and crash the kernel.

> +	put_unaligned_le32(cpu_to_le32(baud), &req.baudrate);

[Severity: Medium]
Is the baud rate being double-swapped on big-endian architectures?

The put_unaligned_le32() macro implicitly handles CPU-to-LE32 conversion
internally. Calling cpu_to_le32(baud) first will swap the bytes on
big-endian platforms, and then put_unaligned_le32() will swap them a second
time, writing the baud rate to the hardware in big-endian format.

[ ... ]

> +static int ft260_uart_install(struct tty_driver *driver, struct tty_struct *tty)
> +{
> +	int idx = tty->index;
> +	struct ft260_device *port = ft260_uart_port_get(idx);
> +	int ret = tty_standard_install(driver, tty);
> +
> +	if (ret == 0)
> +		/* This is the ref ft260_uart_port get provided */
> +		tty->driver_data = port;

[Severity: High]
Can this assign a NULL pointer if the device is disconnected before being
opened?

If the port was already removed, ft260_uart_port_get(idx) returns NULL.
This code does not check if port is NULL before assigning it to
tty->driver_data, which allows the open sequence to continue with a NULL
device context.

[ ... ]

> +static int ft260_uart_probe(struct hid_device *hdev, struct ft260_device *dev)
> +{
> +	struct ft260_configure_uart_request req;
> +	int ret;
> +	struct device *devt;

[ ... ]

> +	/* Send Feature Report to Configure FT260 as UART 9600-8-N-1 */
> +	req.report	= FT260_SYSTEM_SETTINGS;
> +	req.request	= FT260_SET_UART_CONFIG;
> +	req.flow_ctrl	= FT260_CFG_FLOW_CTRL_NONE;
> +	put_unaligned_le32(cpu_to_le32(9600), &req.baudrate);

[Severity: Medium]
Is this also double-swapped on big-endian architectures?

[ ... ]

> @@ -1010,20 +1676,26 @@ static int ft260_probe(struct hid_device *hdev, const struct hid_device_id *id)

[ ... ]

> -err_i2c_free:
> -	i2c_del_adapter(&dev->adap);
>  err_hid_close:
>  	hid_hw_close(hdev);
>  err_hid_stop:
>  	hid_hw_stop(hdev);
> +hid_fail:
> +	kfree(dev);
> +alloc_fail:
>  	return ret;
>  }

[Severity: High]
Is it possible to double free the dev structure here?

If ft260_uart_probe() fails, it calls ft260_uart_port_remove(), which
drops the TTY port's reference count to 0, triggering
ft260_uart_port_destroy() and freeing dev.

The error then propagates back to ft260_probe(), which jumps to
err_hid_close and then unconditionally executes kfree(dev) at the
hid_fail label.

> @@ -1100,8 +1755,18 @@ 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->ft260_is_serial) {
> +		// FIXME:
> +		cancel_work_sync(&dev->wakeup_work);
> +		tty_port_unregister_device(&dev->port, ft260_tty_driver,
> +					   dev->index);
> +		ft260_uart_port_remove(dev);
> +		/* dev still needed, so we will free it in _destroy func */

[Severity: High]
Is there a use-after-free race condition during device unplug?

If the timer fires after cancel_work_sync(&dev->wakeup_work) but before
timer_delete_sync(&port->wakeup_timer) runs in ft260_uart_port_remove(),
the timer callback ft260_uart_start_wakeup() can observe
dev->reschedule_work == true and dynamically re-queue the work.

The work would then remain queued after the device is destroyed.

> +	} else {
> +		sysfs_remove_group(&hdev->dev.kobj, &ft260_attr_group);
> +		i2c_del_adapter(&dev->adap);
> +		kfree(dev);
> +	}
>  
>  	hid_hw_close(hdev);
>  	hid_hw_stop(hdev);

[Severity: High]
Does this free dev while the HID hardware is still active?

By calling kfree(dev) before hid_hw_stop(hdev), incoming hardware reports
can still be dispatched to ft260_raw_event(). That handler calls
hid_get_drvdata(hdev) and will dereference the freed dev structure.

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