Re: [PATCH v2 02/15] HID: ft260: uart: fix the serial driver

[sashiko-bot@kernel.org]

Thank you for your contribution! Sashiko AI review found 9 potential issue(s) to consider:
- [High] HID: ft260: invalid report ID 0xFF in ft260_uart_transmit_chars()
- [High] HID: ft260: race and infinite loop in ft260_uart_transmit_chars()
- [High] HID: ft260: wakeup timer and work teardown race in ft260_remove()
- [High] HID: ft260: broken index allocation and bounds check in ft260_uart_add_port()
- [High] HID: ft260: UAF and double free when ft260_get_interface_type() returns 0
- [High] HID: ft260: missing iface_type check for UART reports in ft260_raw_event()
- [Medium] HID: ft260: unconditional wakeup timer in ft260_uart_port_activate()
- [Medium] HID: ft260: ft260_uart_probe() registers TTY device before configuring UART
- [Low] HID: ft260: unneeded ft260_uart_wakeup() prototype and changelog mismatches
--

commit 120d1c2c1f711fdf1a272089095a3cbf1a2d1d34
Author: Michael Zaidman <michael.zaidman@gmail.com>

HID: ft260: uart: fix the serial driver

This commit updates the FT260 serial driver to separate the UART and I2C TX
buffers, adjust the power-save wakeup timer logic, validate HID input report
lengths against the received report size, and clean up interface detection and
FIFO handling.

>     HID: ft260: uart: fix the serial driver
>     
>     Fix the serial driver added by the previous patch, a port of
>     Christina Quast's v4, so that it works reliably on hardware.
>     
>     The UART and I2C paths shared write_buf without proper separation,
>     so concurrent use could corrupt transfers on both channels. Give
>     UART its own TX buffer. The write request report is now built in
>     that fixed per-port buffer, so its payload member becomes a fixed
>     FT260_WR_UART_DATA_MAX (62) array rather than a __counted_by()
>     flexible array, which describes a runtime-counted allocation this
>     path no longer makes.
>     
>     The FT260 enters power-save after ~5s idle, which drops RX data at
>     higher baud rates. Keep a timed dummy wakeup, but only when needed:
>     skip it below 4800 baud and when EEPROM disables power-save; wake
>     earlier before RX; avoid configuring baud or disabling the workaround
>     twice; and arm the timer when a tty session starts.
>     
>     The received report length was validated against the actual HID report
>     size only on the I2C path. The UART path instead bounded the
>     device-supplied length against FT260_RD_DATA_MAX (180), which is the
>     maximum I2C read payload, not a bound on the report buffer. As a UART
>     payload cannot exceed FT260_WR_UART_DATA_MAX (62) bytes, a report
>     claiming more would make ft260_uart_receive_chars() push data past the
>     end of the report buffer into the tty flip buffer. Validate the length
>     against the received report size once, before dispatching on the report
>     type, and drop the two now-redundant FT260_RD_DATA_MAX checks.
>     
>     Also fix RX data loss after reopening the device, improve TX
>     throughput, simplify HID interface-type detection, use kfifo_avail
>     for write-room, and ignore spurious UART interrupt status (0xb1)
>     reports instead of treating them as errors.
>     
>     Also includes UART cleanup and refactoring to match the rest of the
>     driver style: replace the kfifo_*_locked aliases with
>     kfifo_*_spinlocked, and drop the parentheses around single-value
>     defines, which coding-style asks for only when the value is an
>     expression.
>     
>     This replaces the UART bring-up fix series:
>     https://lore.kernel.org/all/20240210215147.77629-1-michael.zaidman@gmail.com/
>     
>     Do not treat UART interrupt status (0xB1) as a payload length. When
>     re-enabling the power-save wakeup workaround after a baud change,
>     re-arm the timer (reported by Sashiko).
>     
>     Report 0xE1 is defined as FT260_UART_RI_DCD_STATUS, but AN_394 section
>     4.3 lists 0xE2 for "UART RI and DCD Status". Correct the value. The
>     macro has no user, because the DCD and RI levels arrive in the 0xB1
>     interrupt report, which ft260_raw_event() handles.
>     
>     Signed-off-by: Michael Zaidman <michael.zaidman@gmail.com>

[Severity: Low]
This isn't a bug in the code, but does the commit message match what the patch
does here?

The changelog mentions waking earlier before RX, but
FT260_WAKEUP_NEEDED_AFTER_MS remains 4800 ms and ft260_uart_wakeup() is only
called from the periodic ft260_uart_do_wakeup() work handler.

The last paragraph also states that DCD and RI levels arrive in the 0xB1
interrupt report, which ft260_raw_event() handles, whereas ft260_raw_event()
ignores FT260_UART_INTERRUPT_STATUS (0xB1) and returns 0.

> diff --git a/drivers/hid/hid-ft260.c b/drivers/hid/hid-ft260.c
> index 982e2ca7a2de6..c859486cb28c8 100644
> --- a/drivers/hid/hid-ft260.c
> +++ b/drivers/hid/hid-ft260.c
[ ... ]
> @@ -38,11 +34,14 @@ MODULE_PARM_DESC(debug, "Toggle FT260 debugging messages");
>  			pr_info("%s: " format, __func__, ##arg);	  \
>  	} while (0)
>  
> -#define FT260_REPORT_MAX_LENGTH (64)
> -#define FT260_I2C_DATA_REPORT_ID(len) (FT260_I2C_REPORT_MIN + (len - 1) / 4)
> -#define FT260_UART_DATA_REPORT_ID(len) (FT260_UART_REPORT_MIN + (len - 1) / 4)
> +#define FT260_REPORT_MAX_LEN 64
> +#define FT260_DATA_REPORT_ID(min, len) (min + (len - 1) / 4)
> +#define FT260_I2C_DATA_REPORT_ID(len) \
> +		FT260_DATA_REPORT_ID(FT260_I2C_REPORT_MIN, len)
> +#define FT260_UART_DATA_REPORT_ID(len) \
> +		FT260_DATA_REPORT_ID(FT260_UART_REPORT_MIN, len)
>  
> -#define FT260_WAKEUP_NEEDED_AFTER_MS (4800) /* 5s minus 200ms margin */
> +#define FT260_WAKEUP_NEEDED_AFTER_MS 4800 /* 5s minus 200ms margin */
>  
>  /*
>   * The ft260 input report format defines 62 bytes for the data payload, but
> @@ -55,8 +54,9 @@ MODULE_PARM_DESC(debug, "Toggle FT260 debugging messages");
>   * of I2C for potentially troublesome amounts of time, we select the maximum
>   * read payload length to be 180 bytes.
>   */
> -#define FT260_RD_DATA_MAX (180)
> -#define FT260_WR_DATA_MAX (60)
> +#define FT260_RD_DATA_MAX 180
> +#define FT260_WR_I2C_DATA_MAX 60
> +#define FT260_WR_UART_DATA_MAX 62

[Severity: High]
When ft260_uart_write() calls ft260_uart_transmit_chars() with 61 or more
bytes in xmit_fifo, len is clamped to FT260_WR_UART_DATA_MAX (62):

    len = min(data_len, FT260_WR_UART_DATA_MAX);
    rep->report = FT260_UART_DATA_REPORT_ID(len);

For len = 61 or 62, (len - 1) / 4 evaluates to 15, so
FT260_UART_DATA_REPORT_ID(len) produces 0xF0 + 15 = 0xFF.

Since the valid UART data report IDs end at FT260_UART_REPORT_MAX (0xFE, which
carries 57 to 62 payload bytes), won't setting rep->report to 0xFF cause
transfers of 61 or 62 bytes to fail with an invalid HID report ID?

[ ... ]
> @@ -937,27 +951,25 @@ static int ft260_get_interface_type(struct hid_device *hdev, struct ft260_device
>  	ft260_dbg("i2c_enable: 0x%02x\n", cfg.i2c_enable);
>  	ft260_dbg("uart_mode:  0x%02x\n", cfg.uart_mode);
>  
> -	dev->ft260_is_serial = false;
> +	dev->power_saving_en = cfg.power_saving_en;
>  
>  	switch (cfg.chip_mode) {
>  	case FT260_MODE_ALL:
>  	case FT260_MODE_BOTH:
> -		if (interface == 1) {
> +		if (interface == 1)
>  			ret = FT260_IFACE_UART;
> -			dev->ft260_is_serial = true;
> -		} else {
> +		else
>  			ret = FT260_IFACE_I2C;
> -		}
>  		break;
>  	case FT260_MODE_UART:
>  		ret = FT260_IFACE_UART;
> -		dev->ft260_is_serial = true;
>  		break;
>  	case FT260_MODE_I2C:
>  		ret = FT260_IFACE_I2C;
>  		break;
>  	}
>  
> +	dev->iface_type = ret;
>  	return ret;
>  }

[Severity: High]
If cfg.chip_mode does not match any of the four cases in
ft260_get_interface_type(), ret remains 0 (from ft260_get_system_config()),
which equals FT260_IFACE_NONE.

In ft260_probe(), hid_set_drvdata(hdev, dev) is called before
ft260_get_interface_type(), and the check:

    ret = ft260_get_interface_type(hdev, dev);
    if (ret <= FT260_IFACE_NONE)
        goto err_hid_close;

jumps to err_hid_close with ret == 0. That error path frees dev via kfree(dev)
and returns 0 from ft260_probe().

Won't returning 0 cause the driver core to treat probe as successful, leading
to a use-after-free and double free of dev when ft260_remove() later retrieves
dev from hid_get_drvdata(hdev)?

[ ... ]
> @@ -1101,12 +1113,36 @@ static const struct attribute_group ft260_attr_group = {
>  	}
>  };
>  
> -/***
> - * START Serial dev part
> - */
>  static DEFINE_MUTEX(ft260_uart_list_lock);
>  static LIST_HEAD(ft260_uart_device_list);
>  
> +static void ft260_uart_wakeup(struct ft260_device *dev);

[Severity: Low]
This isn't a bug, but is this forward declaration of ft260_uart_wakeup()
needed? Its only caller is ft260_uart_do_wakeup(), which is defined right
below ft260_uart_wakeup().

[ ... ]
> @@ -1123,8 +1159,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);

[Severity: High]
In ft260_uart_add_port(), the minor index search assumes
ft260_uart_device_list is sorted by index, but new ports are appended to the
tail of the list without checking UART_COUNT_MAX:

    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);

If ports 0 and 1 are added, port 0 is removed, and a new port A is probed,
port A gets index 0 and is placed at the tail (making the list order [1, 0]).
If another port B is then probed, the first entry has dev->index == 1 != 0, so
the loop breaks immediately and assigns index 0 to port B as well.

When ft260_uart_probe() then calls tty_port_register_device_attr(),
tty_port_link_device() overwrites ft260_tty_driver->ports[0] with &devB->port
before tty_register_device_attr() fails with -EEXIST and frees devB, leaving
ft260_tty_driver->ports[0] dangling for ft260_uart_install() ->
tty_standard_install(). Also, if more than UART_COUNT_MAX (4) ports are
probed, index >= UART_COUNT_MAX triggers WARN_ON(index >= driver->num) in
tty_port_link_device().

Should ft260_uart_add_port() insert before dev->device_list to keep the list
sorted and return an error when index >= UART_COUNT_MAX?

[ ... ]
> @@ -1217,30 +1253,24 @@ static int ft260_uart_transmit_chars(struct ft260_device *port)
>  		goto tty_out;
>  	}
>  
> -	rep = (struct ft260_uart_write_request_report *)port->write_buf;
> +	rep = (struct ft260_uart_write_request_report *)port->uart_wr_buf;
>  
>  	do {
> -		len = min(data_len, FT260_WR_DATA_MAX);
> +		len = min(data_len, FT260_WR_UART_DATA_MAX);
>  
>  		rep->report = FT260_UART_DATA_REPORT_ID(len);
>  		rep->length = len;
>  
> -		len = kfifo_out_locked(xmit, rep->data, len, &port->write_lock);
> +		len = kfifo_out_spinlocked(xmit, rep->data, len, &port->xmit_fifo_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);
> +		ret = ft260_hid_output_report(hdev, (u8 *)rep, len + 2);
> +		if (ret < 0)
>  			goto tty_out;
> -		}
>  
>  		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:

[Severity: High]
As reported by Sashiko on v1 [1], ft260_uart_transmit_chars() builds outgoing
reports in the shared per-port buffer port->uart_wr_buf without holding a lock
across the loop while ft260_hid_output_report() sleeps.

If two threads enter ft260_uart_write() -> ft260_uart_transmit_chars()
concurrently (or if kfifo_reset() runs in ft260_uart_port_activate()):

1. Both threads can overwrite port->uart_wr_buf concurrently before kmemdup()
   in ft260_hid_output_report() copies it.
2. Because rep->report and rep->length are set from data_len before
   kfifo_out_spinlocked(), a racing thread draining xmit_fifo can cause
   kfifo_out_spinlocked() to return fewer bytes than rep->length, or 0 bytes.
3. If kfifo_out_spinlocked() returns 0, ft260_hid_output_report() sends a
   2-byte report with a non-zero rep->length and data_len -= 0 never
   terminates the while (data_len > 0) loop.

Also, with tty_wakeup(tty) removed here, what wakes up a writer waiting for
write room after xmit_fifo fills up?

[ ... ]
> @@ -1503,53 +1537,68 @@ static void ft260_uart_start_wakeup(struct timer_list *t)
[ ... ]
> +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) {
> -		mod_timer(&port->wakeup_timer, jiffies +
> -			  msecs_to_jiffies(FT260_WAKEUP_NEEDED_AFTER_MS));
> -	}
> +	/*
> +	 * 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);
> +	if (ret)
> +		return ret;
> +
> +	baudrate = get_unaligned_le32(&cfg.baudrate);
> +	if (baudrate > FT260_UART_EN_PW_SAVE_BAUD)
> +		ft260_uart_wakeup_workaraund_enable(port, true);
> +
> +	ft260_dbg("configured baudrate = %d", baudrate);
> +
> +	mod_timer(&port->wakeup_timer, jiffies +
> +		  msecs_to_jiffies(FT260_WAKEUP_NEEDED_AFTER_MS));
>  
>  	return 0;
>  }

[Severity: Medium]
In ft260_uart_port_activate(), mod_timer(&port->wakeup_timer, ...) is now
called unconditionally rather than only when port->reschedule_work is true.
Also, if baudrate <= FT260_UART_EN_PW_SAVE_BAUD on the first open after
ft260_uart_probe() enabled the workaround, port->reschedule_work remains true.

Should ft260_uart_port_activate() pass baudrate > FT260_UART_EN_PW_SAVE_BAUD
directly to ft260_uart_wakeup_workaraund_enable() and guard mod_timer() with
if (port->reschedule_work), as in ft260_uart_change_speed()?

[ ... ]
> @@ -1641,10 +1689,10 @@ static int ft260_uart_probe(struct hid_device *hdev, struct ft260_device *dev)
>  		ret = PTR_ERR(devt);
>  		goto err_register_tty;
>  	}
> -	hid_info(hdev, "Registering device /dev/%s%d\n",
> +	hid_info(hdev, "registering device /dev/%s%d\n",
>  		ft260_tty_driver->name, dev->index);
>  
> -	/* Send Feature Report to Configure FT260 as UART 9600-8-N-1 */
> +	/* Configure UART to 9600n8 */
>  	req.report	= FT260_SYSTEM_SETTINGS;
>  	req.request	= FT260_SET_UART_CONFIG;
>  	req.flow_ctrl	= FT260_UART_CFG_FLOW_CTRL_NONE;
> @@ -1656,8 +1704,7 @@ static int ft260_uart_probe(struct hid_device *hdev, struct ft260_device *dev)
>  
>  	ret = ft260_hid_feature_report_set(hdev, (u8 *)&req, sizeof(req));
>  	if (ret < 0) {
> -		hid_err(hdev, "ft260_hid_feature_report_set failed: %d\n",
> -			ret);
> +		hid_err(hdev, "failed to configure uart: %d\n", ret);
>  		goto err_hid_report;
>  	}

[Severity: Medium]
In ft260_uart_probe(), tty_port_register_device_attr() exposes /dev/ttyFT* to
userspace before ft260_hid_feature_report_set() configures the UART to 9600n8.

Since ft260_uart_port_activate() now reads the active UART settings from the
chip via ft260_get_uart_settings() instead of calling
ft260_uart_change_speed(), can a userspace process opening the port
immediately after registration read uninitialized UART settings or have its
termios changes overwritten by the 9600n8 feature report in
ft260_uart_probe()?

[ ... ]
> @@ -1765,7 +1812,7 @@ static void ft260_remove(struct hid_device *hdev)
>  	if (!dev)
>  		return;
>  
> -	if (dev->ft260_is_serial) {
> +	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,
> @@ -1779,7 +1826,7 @@ static void ft260_remove(struct hid_device *hdev)
>  	hid_hw_close(hdev);
>  	hid_hw_stop(hdev);
>  
> -	if (dev->ft260_is_serial)
> +	if (dev->iface_type == FT260_IFACE_UART)
>  		ft260_uart_port_put(dev);
>  	else
>  		kfree(dev);

[Severity: High]
As reported by Sashiko on v1 [2], is the teardown ordering of wakeup_timer and
wakeup_work safe against concurrent TTY operations during removal?

In ft260_remove() and ft260_uart_port_remove():

    ft260_remove()
        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)
            timer_delete_sync(&port->wakeup_timer);
            list_del(&port->device_list);
            tty_port_tty_vhangup(&port->port);

Because port->reschedule_work is only cleared when tty_port_tty_vhangup()
invokes ft260_uart_port_shutdown():

1. If wakeup_timer fires between cancel_work_sync() in ft260_remove() and
   timer_delete_sync() in ft260_uart_port_remove(), ft260_uart_start_wakeup()
   schedules wakeup_work, which is never cancelled afterwards (and the
   err_hid_report error path in ft260_uart_probe() never calls
   cancel_work_sync() at all).
2. Until list_del() and tty_port_tty_vhangup() complete, a concurrent
   ft260_uart_change_speed() or ft260_uart_port_activate() can set
   port->reschedule_work = true and re-arm wakeup_timer via mod_timer() after
   timer_delete_sync() has already run, leaving the timer or work active after
   hid_hw_stop() and ft260_uart_port_put() free dev.

Should timer_delete_sync(&port->wakeup_timer) and
cancel_work_sync(&port->wakeup_work) happen after list_del() and
tty_port_tty_vhangup() have finished shutting down the port?

> @@ -1797,6 +1844,13 @@ static int ft260_raw_event(struct hid_device *hdev, struct hid_report *report,
[ ... ]
> @@ -1837,12 +1884,11 @@ static int ft260_raw_event(struct hid_device *hdev, struct hid_report *report,
>  
>  		return 0;
>  
> -	} else if (xfer->length > FT260_RD_DATA_MAX) {
> -		hid_err(hdev, "received data too long (%d)\n", xfer->length);
> -		return -EBADR;
>  	} else if (xfer->report >= FT260_UART_REPORT_MIN &&
>  		   xfer->report <= FT260_UART_REPORT_MAX) {
>  		return ft260_uart_receive_chars(dev, xfer->data, xfer->length);
> +	} else if (xfer->report == FT260_UART_INTERRUPT_STATUS) {
> +		return 0;
>  	}
>  	hid_err(hdev, "unhandled report %#02x\n", xfer->report);

[Severity: High]
In ft260_probe(), tty_port_init(&dev->port) is only called on the UART path in
ft260_uart_probe(). When dev->iface_type == FT260_IFACE_I2C, dev->port is left
zero-initialized with dev->port.buf.tail == NULL.

If an input report in the range FT260_UART_REPORT_MIN..FT260_UART_REPORT_MAX
arrives on an I2C interface, ft260_raw_event() calls
ft260_uart_receive_chars() -> tty_insert_flip_string(&port->port, ...), which
dereferences port->port.buf.tail in __tty_buffer_request_room().

Should ft260_raw_event() check that dev->iface_type == FT260_IFACE_UART before
dispatching UART reports to ft260_uart_receive_chars()?

[1] https://lore.kernel.org/r/20260822215615.2D8541F000E9@smtp.kernel.org
[2] https://lore.kernel.org/r/20260822215615.2D8541F000E9@smtp.kernel.org

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