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

[sashiko-bot@kernel.org]

Thank you for your contribution! Sashiko AI review found 3 potential
issues to consider:
- [High] HID: ft260: UAF on port->hdev and premature xmit_fifo free on
  disconnect
- [High] HID: ft260: use-after-free in ft260_uart_activate() on remove
  race
- [Medium] HID: ft260: missing UART_COUNT_MAX check in
  ft260_uart_add_port()
--

commit 0485244a5cc12377d97264627f49ff95e6a060c7
Author: Christina Quast <contact@christina-quast.de>

HID: ft260: add serial driver

This commit adds a TTY serial driver (/dev/ttyFTx) for the UART interface
of FT260 USB HID devices.

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

[ ... ]

> @@ -1001,51 +1101,478 @@ 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_tail(&port->device_list, &ft260_uart_device_list);
> +	mutex_unlock(&ft260_uart_list_lock);
> +
> +	return ret;
> +}

[Severity: Medium]
Does ft260_uart_add_port() need to check if index >= UART_COUNT_MAX before
adding the port?  When a fifth FT260 UART interface is probed,
ft260_uart_probe() passes dev->index == 4 to
tty_port_register_device_attr(), which triggers a WARN_ON(index >=
driver->num) in tty_port_link_device() because ft260_tty_driver was
allocated with UART_COUNT_MAX (4) lines.

While commit "HID: ft260: uart: fix the serial driver" fixes the unsorted
list insertion here, the missing UART_COUNT_MAX bounds check remains at
the end of the series.

> +
> +static void ft260_uart_port_put(struct ft260_device *port)
> +{
> +	tty_port_put(&port->port);
> +}
> +
> +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);
> +
> +	tty_port_tty_vhangup(&port->port);
> +
> +	spin_lock(&port->write_lock);
> +	kfifo_free(&port->xmit_fifo);
> +	spin_unlock(&port->write_lock);
> +}

[Severity: High]
Can freeing port->xmit_fifo in ft260_uart_port_remove() and leaving
port->hdev accessible after ft260_remove() cause a use-after-free?

struct ft260_device stays alive until the final tty_port reference is
dropped in ft260_uart_cleanup() -> ft260_uart_port_destroy(), whereas
ft260_remove() calls ft260_uart_port_remove(), hid_hw_close(hdev), and
hid_hw_stop(hdev), after which usbhid_disconnect() -> hid_destroy_device()
frees hdev.  If a process keeps an open file descriptor across
ft260_remove() and reopens it (for example via /proc/self/fd/<fd>),
tty_open_by_driver() takes the tty_reopen() path into tty_port_open() ->
ft260_uart_activate(), which resets the already freed port->xmit_fifo and
calls ft260_uart_change_speed() -> ft260_hid_feature_report_set() on the
freed port->hdev.  Because ft260_uart_activate() also ignores the return
value of ft260_uart_change_speed() and clears TTY_IO_ERROR, the reopen
succeeds and subsequent ft260_uart_write() and ft260_uart_set_termios()
calls operate on the freed FIFO and freed hdev as well.

Commit "HID: ft260: uart: fix the serial driver" and commit "HID: ft260:
uart: add modem pins control via ioctl" add a port->removed flag for the
modem control ioctls, but ft260_uart_port_activate() and
ft260_uart_change_speed() still do not check port->removed and
port->xmit_fifo is still freed in ft260_uart_port_remove() rather than
ft260_uart_port_destroy().

[ ... ]

> +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;
> +
> +	if (!port)
> +		return -ENODEV;
> +
> +	ret = tty_standard_install(driver, tty);
>  	if (ret) {
> +		ft260_uart_port_put(port);
>  		return ret;
>  	}
>  
> +	tty->driver_data = port;
> +	return 0;
> +}

[ ... ]

> +static int ft260_uart_activate(struct tty_port *tport, struct tty_struct *tty)
> +{
> +	struct ft260_device *port =
> +		container_of(tport, struct ft260_device, port);
> +
> +	/*
> +	 * Set the TTY IO error marker - we will only clear this
> +	 * once we have successfully opened the port.
> +	 */
> +	set_bit(TTY_IO_ERROR, &tty->flags);
> +
> +	spin_lock(&port->write_lock);
> +	kfifo_reset(&port->xmit_fifo);
> +	spin_unlock(&port->write_lock);
> +
> +	ft260_uart_change_speed(port, &tty->termios, NULL);
> +	clear_bit(TTY_IO_ERROR, &tty->flags);
> +
> +	if (port->reschedule_work) {
> +		mod_timer(&port->wakeup_timer, jiffies +
> +			  msecs_to_jiffies(FT260_WAKEUP_NEEDED_AFTER_MS));
> +	}
> +
> +	return 0;
> +}

[Severity: High]
What prevents ft260_uart_activate() from running after ft260_remove() when
tty_open() races with disconnect?

CPU0                                  CPU1
tty_open()
  tty_init_dev()
    ft260_uart_install()
      ft260_uart_port_get()
      tty->driver_data = port
                                      usbhid_disconnect()
                                        hid_destroy_device()
                                          ft260_remove()
                                            ft260_uart_port_remove()
                                              list_del(&port->device_list)
                                              tty_port_tty_vhangup(&port->port)
                                              kfifo_free(&port->xmit_fifo)
                                            hid_hw_close(hdev)
                                            hid_hw_stop(hdev)
                                          kfree(hdev)
  ft260_uart_open()
    tty_port_open()
      tty_port_tty_set()
      ft260_uart_activate()
        kfifo_reset(&port->xmit_fifo)
        ft260_uart_change_speed()
          ft260_hid_feature_report_set(port->hdev, ...)
                                      <-- use-after-free on port->hdev

Between ft260_uart_install() and tty_port_open(), port->port.tty is still
NULL, so tty_port_tty_vhangup() in ft260_uart_port_remove() sees no tty to
hang up.  tty_open() then enters ft260_uart_activate(), resets the freed
port->xmit_fifo, dereferences the freed port->hdev, and clears
TTY_IO_ERROR so the open succeeds on the removed device.  Even after commit
"HID: ft260: uart: fix the serial driver" and commit "HID: ft260: uart:
add modem pins control via ioctl", ft260_uart_port_activate() still does
not check whether the device was removed under a lock synchronized with
disconnect.

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