Re: [PATCH 5/6] USB: serial: mxuport: handle TCFLSH buffer purge

[sashiko-bot@kernel.org]

Thank you for your contribution! Sashiko AI review found 2 potential issue(s) to consider:
- [High] tty: mxuport: TCFLSH interception causes AB-BA deadlock and job control bypass
- [Medium] usb: serial: mxuport: sent_payload accounting desynchronized by in-flight bulk URBs during purge
--

commit 50a2e5dd5f4f9804e6a3238f55deca09791f0018
Author: Crescent Hsieh <crescentcy.hsieh@moxa.com>

USB: serial: mxuport: handle TCFLSH buffer purge

Generic tty flush handling does not purge the receive and transmit
buffers in mxuport firmware or clear the USB serial write FIFO. This
commit handles input, output, and combined flush requests by issuing
the corresponding firmware purge request.

> diff --git a/drivers/usb/serial/mxuport.c b/drivers/usb/serial/mxuport.c
> index 326a5ce18c6a0..9d304fb3aa02f 100644
> --- a/drivers/usb/serial/mxuport.c
> +++ b/drivers/usb/serial/mxuport.c
[ ... ]
> @@ -952,6 +958,70 @@ static int mxuport_set_rs485_config(struct usb_serial_port *port,
>  	return 0;
>  }
>  
> +static void mxuport_tx_flush_start(struct usb_serial_port *port)
> +{
> +	struct mxuport_port *mxport = usb_get_serial_port_data(port);
> +	unsigned long flags;
> +
> +	spin_lock_irqsave(&port->lock, flags);
> +	mxport->hold_reason |= MX_WAIT_FOR_PURGE;
> +	kfifo_reset_out(&port->write_fifo);
> +	spin_unlock_irqrestore(&port->lock, flags);
> +}
> +
> +static void mxuport_tx_flush_done(struct usb_serial_port *port, bool purged)
> +{
> +	struct mxuport_port *mxport = usb_get_serial_port_data(port);
> +	unsigned long flags;
> +
> +	spin_lock_irqsave(&port->lock, flags);
> +	if (purged) {
> +		mxport->sent_payload = 0;
> +		mxport->hold_reason &= ~MX_WAIT_FOR_SEND_NEXT;
> +	}

[Severity: Medium]
Since kfifo_reset_out() empties the software FIFO but does not unlink or
kill already-submitted URBs, is it possible for a late-arriving bulk URB to
be received by the device after the device has processed the RQ_VENDOR_PURGE
control message?

If the host controller schedules the bulk and control transfers such that an
in-flight bulk URB arrives post-purge, resetting sent_payload to 0 here
could permanently desynchronize the driver's flow control accounting. Could
this lead to miscalculating the UPORT_REQUEST_SEND_NEXT threshold, resulting
in over-transmission and an overflow of the device's internal RX buffer?

> +	mxport->hold_reason &= ~MX_WAIT_FOR_PURGE;
> +	spin_unlock_irqrestore(&port->lock, flags);
> +
> +	usb_serial_generic_write_start(port, GFP_KERNEL);
> +}
> +
> +static int mxuport_flush_buffer(struct tty_struct *tty,
> +				unsigned long arg)
> +{
> +	struct usb_serial_port *port = tty->driver_data;
> +	int err, ret;
> +	u16 mask;
> +
> +	switch (arg) {
> +	case TCIFLUSH:
> +		mask = MX_PURGE_RX;
> +		break;
> +	case TCOFLUSH:
> +		mask = MX_PURGE_TX;
> +		break;
> +	case TCIOFLUSH:
> +		mask = MX_PURGE_RX | MX_PURGE_TX;
> +		break;
> +	default:
> +		return -EINVAL;
> +	}
> +
> +	if (mask & MX_PURGE_TX)
> +		mxuport_tx_flush_start(port);
> +
> +	err = mxuport_send_ctrl_urb(port->serial, RQ_VENDOR_PURGE, mask,
> +				    port->port_number);
> +	ret = tty_perform_flush(tty, arg);

[Severity: High]
Does calling tty_perform_flush() from within this driver's ioctl handler
create an AB-BA deadlock between tport->mutex and ldisc_sem?

When ioctl(TCFLSH) is called, tty_ioctl() delegates to serial_ioctl() which
acquires tport->mutex, and then calls mxuport_ioctl() and eventually this
function. tty_perform_flush() acquires ldisc_sem (read).

Concurrently, another thread could call a TCSETS ioctl, which acquires
ldisc_sem (read) in tty_mode_ioctl() and then tries to acquire tport->mutex
via serial_set_termios(). If a writer (like tty_set_ldisc) is waiting for
ldisc_sem, the read lock will block, completing the deadlock.

Additionally, because the hardware buffer is purged here before
tty_perform_flush() calls tty_check_change(), does this bypass POSIX job
control, allowing background processes to evade SIGTTOU restrictions?

> +
> +	if (mask & MX_PURGE_TX)
> +		mxuport_tx_flush_done(port, !err);
> +
> +	if (err)
> +		return err;
> +
> +	return ret;
> +}
> +
>  static int mxuport_ioctl(struct tty_struct *tty, unsigned int cmd,
>  			 unsigned long arg)
>  {
> @@ -959,6 +1029,8 @@ static int mxuport_ioctl(struct tty_struct *tty, unsigned int cmd,
>  	void __user *argp = (void __user *)arg;
>  
>  	switch (cmd) {
> +	case TCFLSH:
> +		return mxuport_flush_buffer(tty, arg);
>  	case TIOCGRS485:
>  		return mxuport_get_rs485_config(port, argp);
>  	case TIOCSRS485:

-- 
Sashiko AI review · https://sashiko.dev/#/patchset/20261016100351.681390-1-crescentcy.hsieh@moxa.com?part=5