Re: [PATCH v6 8/9] serial: max310x: drive RTS in software when hardware delays are too short

[sashiko-bot@kernel.org]

Thank you for your contribution! Sashiko AI review found 3 potential issue(s) to consider:
- [High] serial: max310x: start_tx() unconditionally rewinds active RTS envelope
- [High] serial: max310x: UAF due to timer resurrection bypassing tx_teardown interlock
- [Medium] serial: max310x: TOCTOU race in RTS control leaves pin permanently asserted
--

commit 0a0721a468435345830de47dd7875adbf5a41cb4
Author: Tapio Reijonen <tapio.reijonen@vaisala.com>

serial: max310x: drive RTS in software when hardware delays are too short

This commit adds a software-driven RTS path for the max310x driver to handle
requested RTS delays that exceed what the hardware can natively time. It
uses an hrtimer and workqueues to toggle the RTS line and control transmission
timing.

> diff --git a/drivers/tty/serial/max310x.c b/drivers/tty/serial/max310x.c
> --- a/drivers/tty/serial/max310x.c
> +++ b/drivers/tty/serial/max310x.c
[ ... ]
> +static void max310x_rts_work_proc(struct work_struct *ws)
> +{
> +	struct max310x_one *one = container_of(ws, struct max310x_one, rts_work);
> +	struct uart_port *port = &one->port;
> +	bool rts_on = READ_ONCE(one->tx_state) != MAX310X_TX_OFF ||
> +		      READ_ONCE(one->tx_break);
> +
> +	max310x_rts_ctl(port, max310x_rts_level(port, rts_on));

[Severity: Medium]
Is there a race condition in this lockless state check that could leave the
RTS pin permanently asserted?

If max310x_rts_work_proc executes concurrently with an rs485_config ioctl
that disables RS485, max310x_rs485_config clears tx_state to MAX310X_TX_OFF
but does not cancel any already-queued rts_work.

Could an already-queued rts_work evaluate rts_on based on an older tx_state,
or mistakenly compute an active level because SER_RS485_RTS_AFTER_SEND flags
still linger, and then overwrite the register?

Since RS485 is disabled, this would leave the transceiver permanently driving
the bus.

[ ... ]
> +static void max310x_delayed_start_tx(struct uart_port *port)
> +{
> +	struct max310x_one *one = to_max310x_port(port);
> +
> +	WRITE_ONCE(one->tx_state, MAX310X_TX_WAIT_BEFORE_SEND);
> +	one->cancel_tx_delay_tmr = false;
> +	schedule_work(&one->rts_work);
> +}
> +
> +/* called with port.lock taken and irqs off */
>  static void max310x_start_tx(struct uart_port *port)
>  {
>  	struct max310x_one *one = to_max310x_port(port);
[ ... ]
> +		if (unlikely(res == -1)) {
> +			one->cancel_tx_delay_tmr = true;
> +			uart_port_unlock(port);
> +			hrtimer_cancel(&one->tx_delay_tmr);
> +			uart_port_lock(port);
> +			/*
> +			 * The lock was dropped: a teardown may have run to
> +			 * completion meanwhile. Re-check before starting.
> +			 */
> +			if (one->tx_teardown)
> +				return;
> +		}
> +
> +		max310x_delayed_start_tx(port);
> +	} else {
> -	schedule_work(&one->tx_work);
> +		schedule_work(&one->tx_work);
> +	}
>  }

[Severity: High]
Does this unconditionally rewind an active RTS envelope?

When new data is written to the UART port ring buffer during an active
transmission, max310x_start_tx is called by the serial core while tx_state
is already MAX310X_TX_SEND.

By calling max310x_delayed_start_tx unconditionally, the state is forced
back to MAX310X_TX_WAIT_BEFORE_SEND. The scheduled rts_work will then
incorrectly re-arm tx_delay_tmr with delay_rts_before_send, even though
data is already shifting out.

Because the state is falsely set to MAX310X_TX_WAIT_BEFORE_SEND, the
TX-empty interrupt handler's attempt to arm the after-send hold via
max310x_delayed_stop_tx will abort.

Does start_tx need to check if the envelope is already active in the
MAX310X_TX_SEND phase, and simply schedule tx_work instead of resetting
the envelope?

[ ... ]
> +static bool max310x_adopt_hw_tx(struct max310x_one *one)
> +{
> +	struct uart_port *port = &one->port;
> +
> +	if (!max310x_port_read(port, MAX310X_TXFIFOLVL_REG) &&
> +	    kfifo_is_empty(&port->state->port.xmit_fifo))
> +		return false;
> +
> +	scoped_guard(spinlock_irqsave, &port->lock) {
> +		if (READ_ONCE(one->tx_state) != MAX310X_TX_OFF)
> +			return true;
> +		WRITE_ONCE(one->tx_state, MAX310X_TX_SEND);
> +		one->cancel_tx_delay_tmr = false;
> +	}
> +
> +	return true;
> +}

[Severity: High]
Does this logic introduce a use-after-free risk by resurrecting the timer
after the port has been shut down?

If a concurrent ioctl queues rs_work right after max310x_remove cancels it:

max310x_remove()
    cancel_work_sync(&s->p[i].rs_work);

Since the UART port is unregistered after these cancellations, a concurrent
TIOCSRS485 operation can still queue rs_work. If this delayed rs_work
executes after max310x_shutdown sets the teardown interlocks, it will call
max310x_adopt_hw_tx.

Here, max310x_adopt_hw_tx unconditionally sets cancel_tx_delay_tmr to false
without checking if tx_teardown is true.

Could this arm tx_delay_tmr on the device structure after it has been shut
down, causing a use-after-free when the timer fires on the freed structure?

This problem wasn't introduced by this patch, as the sub-optimal teardown
ordering in max310x_remove is pre-existing, but this bypass of the teardown
interlock in max310x_adopt_hw_tx appears to create a new crash vector.

-- 
Sashiko AI review · https://sashiko.dev/#/patchset/20261004-max310x-rs485-sw-delay-v6-0-3a0ef13ed9e3@vaisala.com?part=8