Re: [PATCH v3 02/16] HID: ft260: uart: fix the serial driver

[sashiko-bot@kernel.org]

Thank you for your contribution! Sashiko AI review found 4 potential
issues to consider:
- [High] HID: ft260: undrained xmit_fifo on TX error or remove hangs tty
  drain and vhangup
- [High] HID: ft260: race between tty open/reopen and ft260_remove()
  causes hdev UAF
- [High] HID: ft260: premature list_del() in ft260_uart_port_remove()
  causes index reuse
- [Medium] HID: ft260: missing UART_COUNT_MAX check in
  ft260_uart_add_port()
--

commit 4759b9dbc3d5fe28cb8dc19ab0c19093471e2b92
Author: Michael Zaidman <michael.zaidman@gmail.com>

HID: ft260: uart: fix the serial driver

This commit fixes buffer sharing, power-save wakeup handling, input report
validation, TX work queueing, and port lifecycle management in the FT260
UART driver.

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

[ ... ]

> @@ -1123,8 +1161,8 @@ 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))
> +	spin_lock_init(&port->xmit_fifo_lock);
> +	if (kfifo_alloc(&port->xmit_fifo, XMIT_FIFO_SIZE, GFP_KERNEL))
>  		return -ENOMEM;
>  
>  	mutex_lock(&ft260_uart_list_lock);
> @@ -1135,7 +1173,8 @@ static int ft260_uart_add_port(struct ft260_device *port)
>  	}
>  
>  	port->index = index;
> -	list_add_tail(&port->device_list, &ft260_uart_device_list);
> +	/* Before the first gap, or at the end: the list stays sorted */
> +	list_add_tail(&port->device_list, &dev->device_list);
>  	mutex_unlock(&ft260_uart_list_lock);
>  
>  	return ret;

[Severity: Medium]
Can ft260_uart_add_port() assign port->index >= UART_COUNT_MAX when more
than four FT260 UART interfaces are probed?

ft260_tty_driver is allocated with UART_COUNT_MAX (4) ports, but
ft260_uart_add_port() never checks index against UART_COUNT_MAX before
adding the port and returning 0.  ft260_uart_probe() then passes
dev->index to tty_port_register_device_attr() -> tty_port_link_device(),
which hits WARN_ON(index >= driver->num).

> @@ -1148,7 +1187,12 @@ static void ft260_uart_port_put(struct ft260_device *port)
>  
>  static void ft260_uart_port_remove(struct ft260_device *port)
>  {
> -	timer_delete_sync(&port->wakeup_timer);
> +	/*
> +	 * Shut down, not delete: a tty call before the hangup below
> +	 * must not re-arm the timer.
> +	 */
> +	timer_shutdown_sync(&port->wakeup_timer);
> +	cancel_work_sync(&port->wakeup_work);
>  
>  	mutex_lock(&ft260_uart_list_lock);
>  	list_del(&port->device_list);
>  	mutex_unlock(&ft260_uart_list_lock);

[Severity: High]
Does removing port->device_list in ft260_uart_port_remove() allow a newly
probed FT260 to reuse port->index while an open file descriptor still
keeps the old tty_struct in ft260_tty_driver->ttys[index]?

Until the last file descriptor is closed and release_tty() runs,
ft260_tty_driver->ttys[index] still points to the old hung-up tty_struct.
If the device is re-plugged before that close, ft260_uart_add_port() sees
the index as free and assigns it to the new device.  Opening the new
/dev/ttyFTx then finds the old tty_struct via tty_driver_lookup_tty() and
calls tty_reopen() -> ft260_uart_open() -> ft260_uart_port_activate() on
the old removed ft260_device, dereferencing its stopped and freed
port->hdev.

Moving list_del(&port->device_list) to ft260_uart_port_destroy() would keep
the index reserved until ft260_tty_driver->ttys[index] has been cleared.

[ ... ]

> @@ -1207,61 +1254,51 @@ static int ft260_uart_transmit_chars(struct ft260_device *port)
>  	struct kfifo *xmit = &port->xmit_fifo;
>  	struct tty_struct *tty;
>  	struct ft260_uart_write_request_report *rep;
> -	int len, data_len, ret = 0;
> +	int len, ret = 0;
>  
>  	tty = tty_port_tty_get(&port->port);
> +	if (!tty)
> +		return -EINVAL;
>  
> -	data_len = kfifo_len(xmit);
> -	if (!tty || !data_len) {
> -		ret = -EINVAL;
> -		goto tty_out;
> -	}
> -
> -	rep = (struct ft260_uart_write_request_report *)port->write_buf;
> -
> -	do {
> -		len = min(data_len, FT260_WR_DATA_MAX);
> +	rep = (struct ft260_uart_write_request_report *)port->uart_wr_buf;
>  
> +	while ((len = kfifo_out_spinlocked(xmit, rep->data,
> +					   FT260_WR_UART_DATA_MAX,
> +					   &port->xmit_fifo_lock)) > 0) {
>  		rep->report = FT260_UART_DATA_REPORT_ID(len);
>  		rep->length = len;
>  
> -		len = kfifo_out_locked(xmit, rep->data, len, &port->write_lock);
> -
> -		ret = ft260_hid_output_report(hdev, (u8 *)rep, len + sizeof(*rep));
> -		if (ret < 0) {
> -			hid_err(hdev, "Failed to start transfer, ret %d\n", ret);
> -			goto tty_out;
> -		}
> +		ret = ft260_hid_output_report(hdev, (u8 *)rep, len + 2);
> +		if (ret < 0)
> +			break;
>  
> -		data_len -= len;
>  		port->icount.tx += len;
> -	} while (data_len > 0);
> -
> -	len = kfifo_len(xmit);
> -	if ((FIFO_SIZE - len) > TTY_WAKEUP_WATERMARK)
> -		tty_wakeup(tty);
> -
> -	ret = 0;
> +	}
>  
> -tty_out:
> +	tty_wakeup(tty);
>  	tty_kref_put(tty);
> -	return ret;
> +	return ret < 0 ? ret : 0;
>  }

[Severity: High]
If ft260_hid_output_report() fails when more than 60 bytes are queued in
xmit_fifo, does breaking out of this loop leave xmit_fifo non-empty with
nothing to drain or flush it?

ft260_uart_chars_in_buffer() then continues returning a non-zero
kfifo_len(&port->xmit_fifo) forever, and ft260_uart_ops has no
.flush_buffer callback.  As a result, tcdrain() and
set_termios(TCSADRAIN/TCSAFLUSH) sleep indefinitely in
wait_event_interruptible(tty->write_wait, !tty_chars_in_buffer(tty)), and
if the device is unplugged while set_termios() is waiting with an ldisc
reference held, ft260_uart_port_remove() -> tty_port_tty_vhangup() ->
tty_ldisc_hangup() blocks forever in tty_ldisc_lock() before reaching
kfifo_free(&port->xmit_fifo):

CPU0                                  CPU1
tty_ioctl() [TCSETSW]
  ld = tty_ldisc_ref_wait(tty)
  ...
  set_termios()
    wait_event_interruptible(
      tty->write_wait,
      !tty_chars_in_buffer(tty))
                                      ft260_uart_port_remove()
                                        tty_port_tty_vhangup(&port->port)
                                          ...
                                          tty_ldisc_hangup()
                                            tty_driver_flush_buffer(tty)
                                            wake_up_interruptible_poll(
                                              &tty->write_wait, EPOLLOUT)
    /* xmit_fifo still non-empty,
       goes back to sleep */
                                            tty_ldisc_lock(tty,
                                              MAX_SCHEDULE_TIMEOUT)
                                              <-- hangs forever

[ ... ]

> @@ -1503,53 +1548,69 @@ static void ft260_uart_start_wakeup(struct timer_list *t)

[ ... ]

> -static int ft260_uart_activate(struct tty_port *tport, struct tty_struct *tty)
> +static int ft260_uart_port_activate(struct tty_port *tport, struct tty_struct *tty)
>  {
> +	int ret;
> +	int baudrate;
> +	struct ft260_get_uart_settings_report cfg;
> +	struct ft260_device *port = container_of(tport, struct ft260_device, port);
> +
>  	set_bit(TTY_IO_ERROR, &tty->flags);
>  
> -	spin_lock(&port->write_lock);
> +	spin_lock(&port->xmit_fifo_lock);
>  	kfifo_reset(&port->xmit_fifo);
> -	spin_unlock(&port->write_lock);
> +	spin_unlock(&port->xmit_fifo_lock);
>  
> -	ft260_uart_change_speed(port, &tty->termios, NULL);
>  	clear_bit(TTY_IO_ERROR, &tty->flags);
>  
> -	if (port->reschedule_work) {
> +	/*
> +	 * The port setting may remain intact after session termination.
> +	 * Then, when reopening the port without configuring the port
> +	 * setting, we need to retrieve the baud rate from the device to
> +	 * reactivate the wakeup workaround if needed.
> +	 */
> +	ret = ft260_get_uart_settings(port->hdev, &cfg);

[Severity: High]
Can ft260_uart_port_activate() race with ft260_remove() and dereference
port->hdev after it has been stopped and freed?

CPU0                                  CPU1
tty_open()
  tty_open_by_driver()
    tty_init_dev()
      ft260_uart_install()
        ft260_uart_port_get()
    mutex_unlock(&tty_mutex)
                                      ft260_remove()
                                        ...
                                        ft260_uart_port_remove()
                                          list_del(&port->device_list)
                                          tty_port_tty_vhangup(&port->port)
                                            /* port.tty is still NULL */
                                          kfifo_free(&port->xmit_fifo)
                                        hid_hw_close(hdev)
                                        hid_hw_stop(hdev)
                                        /* hdev freed by HID/USB core */
  ft260_uart_open()
    tty_port_open()
      ft260_uart_port_activate()
        ft260_get_uart_settings(port->hdev, ...)
          <-- use-after-free on port->hdev

The same race also happens on reopen when a second opener waits on
tty_lock_interruptible(tty) in tty_open_by_driver() while
tty_port_tty_vhangup() runs: vhangup clears the tty_port's initialized
flag, and tty_reopen() then proceeds to ft260_uart_open() ->
tty_port_open() -> ft260_uart_port_activate() after ft260_remove() has
finished.

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