Re: [PATCH v6 5/9] serial: max310x: wait for TX to drain before powering down in shutdown

[sashiko-bot@kernel.org]

Thank you for your contribution! Sashiko AI review found 1 potential issue(s) to consider:
- [High] serial: max310x: DoS via uninterruptible sleep in max310x_shutdown()
--

commit 418c89d1dd7ac6d2979824452f69b97e06892699
Author: Tapio Reijonen <tapio.reijonen@vaisala.com>

serial: max310x: wait for TX to drain before powering down in shutdown

This patch waits for the TX FIFO to drain before powering down the port
in max310x_shutdown(), calculating the per-character sleep duration
based on the configured baud rate.

> diff --git a/drivers/tty/serial/max310x.c b/drivers/tty/serial/max310x.c
> index 2456a2af891f5..9dcc817b97945 100644
> --- a/drivers/tty/serial/max310x.c
> +++ b/drivers/tty/serial/max310x.c

[ ... ]

> @@ -1124,10 +1126,13 @@ static void max310x_set_termios(struct uart_port *port,
>  	uart_update_timeout(port, termios->c_cflag, baud);
>  
>  	/*
> -	 * Cache the new baud rate and reprogram the RS485 RTS delays, whose
> -	 * millisecond-to-bit-time conversion depends on it.
> +	 * Cache the new baud rate and the time it takes to clock out one
> +	 * character before reprogramming the RS485 RTS delays: the
> +	 * millisecond-to-bit-time conversion divides by the baud rate.
>  	 */
>  	to_max310x_port(port)->baud = baud;
> +	to_max310x_port(port)->char_time_us =
> +		DIV_ROUND_UP(USEC_PER_SEC * frame_bits, baud);

[Severity: High]
Does this calculation create a dangerously large sleep duration at extremely
low baud rates?

If a user configures a very low baud rate (e.g., 50 baud or even 1 baud), this
value becomes extremely large since it scales inversely with the baud rate.

>  	max310x_set_rts_ctl_params(to_max310x_port(port));
>  }
>  
> @@ -1223,6 +1228,22 @@ static int max310x_startup(struct uart_port *port)
>  
>  static void max310x_shutdown(struct uart_port *port)
>  {
> +	struct max310x_one *one = to_max310x_port(port);
> +	unsigned int tries = port->fifosize + 1;
> +
> +	/*
> +	 * The tty layer's wait-until-sent is bounded by closing_wait, which
> +	 * can be configured to none, and a hangup reaches here with no wait
> +	 * at all, so the FIFO may still hold anything up to its full size.
> +	 * tx_empty() also only reflects the chip TX FIFO - the last character
> +	 * may still be in the transmit shift register. Let the FIFO drain and
> +	 * the final character clock out before the port is powered down,
> +	 * otherwise the transfer is truncated on the wire.
> +	 */
> +	while (!max310x_tx_empty(port) && tries-- > 0)
> +		fsleep(one->char_time_us);
> +	fsleep(one->char_time_us);

[Severity: High]
Can this loop be used to cause a local Denial of Service via an extended
D-state hang or Hung Task Panic?

If a user opens the TTY, configures a very low baud rate (e.g., speed 1-50),
disables CTS flow control (preventing the TX FIFO from draining), and closes
the device, max310x_shutdown() could call fsleep() up to 129 times in
TASK_UNINTERRUPTIBLE.

At 50 baud, char_time_us is 200,000, causing a 25.8 second uninterruptible
sleep per close(). At 1 baud, this sleep duration exceeds 20 minutes, which
would trigger the kernel's hung_task_panic. Even without a panic, the
extended D-state sleep holds the TTY port mutex, blocking system services
like udev or other users from accessing the port.

> +
>  	/* Disable all interrupts */
>  	max310x_port_write(port, MAX310X_IRQEN_REG, 0);
>

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