Re: [PATCH 07/13] HID: ft260: uart: enable flow control

Thank you for your contribution! Sashiko AI review found 5 potential issue(s) to consider:

Pre-existing issues:
- [Critical] The device structure `dev` is freed prematurely during probe failure and device removal, causing a double-free and use-after-free.
- [Critical] `ft260_remove()` tears down driver data structures before stopping the HID transport, violating safe cleanup order.
- [High] Mismanaged asynchronous worker cleanup causes race conditions and Use-After-Free.
- [High] The shared `uart_wr_buf` buffer is used without locking, leading to data corruption.
- [High] The UART baud rate is double-swapped on big-endian architectures, causing astronomically incorrect values.
--

--- Patch [7]: [PATCH 07/13] HID: ft260: uart: enable flow control ---
commit e6afc26d742478ffb1a839b17d94e5df01b28507
Author: Michael Zaidman <[email protected]>

HID: ft260: uart: enable flow control

This patch updates the FT260 driver to honor CRTSCTS and IXON/IXOFF flow
control settings during termios setup. It ensures flow control is not
forced off, which improves reliability for large transfers at high baud rates.

> diff --git a/drivers/hid/hid-ft260.c b/drivers/hid/hid-ft260.c
> index 1ed3701eaaf75..1da42b8756d8c 100644
> --- a/drivers/hid/hid-ft260.c
> +++ b/drivers/hid/hid-ft260.c
> @@ -1868,8 +1868,12 @@ static int ft260_uart_change_speed(struct ft260_device *port,
>  
>  	if (termios->c_cflag & CRTSCTS)
>  		req.flow_ctrl = FT260_UART_CFG_FLOW_CTRL_RTS_CTS;
> +	else if (termios->c_iflag & (IXON | IXOFF))
> +		req.flow_ctrl = FT260_UART_CFG_FLOW_CTRL_XON_XOFF;
>  	else
> -		req.flow_ctrl = FT260_UART_CFG_FLOW_CTRL_OFF;
> +		req.flow_ctrl = FT260_UART_CFG_FLOW_CTRL_NONE;
> +
> +	req.breaking = FT260_UART_CFG_BREAKING_NO;

[Severity: High]
This isn't a bug introduced by this patch, but while reviewing this function,
does the baud rate configuration perform an incorrect byte swap?

Earlier in ft260_uart_change_speed():

    put_unaligned_le32(cpu_to_le32(baud), &req.baudrate);
    
Since put_unaligned_le32() already handles the conversion from CPU-endian to
little-endian natively, does wrapping it in cpu_to_le32() cause a double-swap
on big-endian architectures, resulting in astronomically incorrect baud rates?

>  	ft260_dbg("configured termios: flow control: %d, baudrate: %d, ",
>  		  req.flow_ctrl, baud);
[ ... ]

[Severity: High]
This is a pre-existing issue, but does the UART transmit path have a data
corruption vulnerability when used concurrently?

Looking at ft260_uart_transmit_chars(), the code writes directly to the shared
buffer without a lock:

    rep = (struct ft260_uart_write_request_report *)port->uart_wr_buf;
    ...
    rep->report = FT260_UART_DATA_REPORT_ID(len);

Since ft260_uart_transmit_chars() is called by the TTY write callback
(ft260_uart_write), and the TTY layer can invoke this concurrently from
multiple contexts, will concurrent executions overwrite each other's report
payloads before ft260_hid_output_report() sends them?

[Severity: Critical]
This isn't a bug introduced by this patch, but does the device lifecycle
management contain a double-free and use-after-free vulnerability?

In ft260_probe(), if ft260_uart_probe() fails, it triggers the cleanup:

ft260_probe()
    ...
    ret = ft260_uart_probe(hdev, dev);
    if (ret)
        goto hid_fail;
    ...
hid_fail:
    kfree(dev);

Inside the ft260_uart_probe() failure path or during ft260_remove(),
ft260_uart_port_remove(dev) drops the TTY port reference to 0, triggering
ft260_uart_port_destroy() which frees dev via kfree(port). When execution
reaches hid_fail, does kfree(dev) cause a double-free?

Similarly, in ft260_remove():

ft260_remove()
    ...
    ft260_uart_port_remove(dev);
    /* dev is still needed, so we will free it in _destroy func */
    if (dev->chip_mode & FT260_MODE_UART || ...)

Does evaluating dev->chip_mode immediately after ft260_uart_port_remove()
dereference the already freed dev pointer?

[Severity: High]
This is a pre-existing issue, but is there a race condition with the
asynchronous worker cleanup during device removal?

Looking at ft260_remove():

ft260_remove()
    ...
    cancel_work_sync(&dev->wakeup_work);
    ...
    ft260_uart_port_remove(dev);

The cancel_work_sync() is called before timer_delete_sync() which is executed
downstream inside ft260_uart_port_remove().

If the wakeup_timer fires immediately after cancel_work_sync() completes,
could its callback ft260_uart_start_wakeup() re-schedule the worker? The
subsequent timer_delete_sync() would cancel the timer but not the newly queued
work, allowing the pending worker to access the device structure after it has
been freed.

[Severity: Critical]
This isn't a bug introduced by this patch, but does ft260_remove() tear
down driver data structures before stopping the HID transport?

In ft260_remove():

ft260_remove()
    ...
    ft260_uart_port_remove(dev);
    ...
    hid_hw_stop(hdev);

By freeing the dev structure before calling hid_hw_stop(hdev), the USB
interrupt endpoint remains active. If incoming HID reports arrive
concurrently, will the HID core dispatch these to ft260_raw_event()
which dereferences dev = hid_get_drvdata(hdev), accessing memory that
has already been freed?

-- 
Sashiko AI review · https://sashiko.dev/#/patchset/[email protected]?part=7