Re: [PATCH v2 1/1] serial: core: Don't clobber the baud rate on hangup via B0

[sashiko-bot@kernel.org]

Thank you for your contribution! Sashiko AI review found 2 potential issue(s) to consider:
- [High] tty: division by zero in uart_get_divisor() on uninitialized port->hup_speed
- [Medium] tty: serial: data race on port->hup_speed in uart_get_baud_rate()
--

commit 3c831138bb5fb81f1a02fc66246315af11bf6bbc
Author: Maciej W. Rozycki <macro@orcam.me.uk>

serial: core: Don't clobber the baud rate on hangup via B0

Requesting hangup via the B0 baud rate clobbers the previously set rate
unless it is 9600bps or the port minimum. This commit tracks the most
recently set baud rate in port->hup_speed and uses it whenever the B0
rate is requested.

> diff --git a/drivers/tty/serial/serial_core.c b/drivers/tty/serial/serial_core.c
> index 319a4b427f3a3..3390e4d142127 100644
> --- a/drivers/tty/serial/serial_core.c
> +++ b/drivers/tty/serial/serial_core.c
[ ... ]
> @@ -514,10 +514,12 @@ uart_get_baud_rate(struct uart_port *port, struct ktermios *termios,
>  		 * Special case: B0 rate.
>  		 */
>  		if (baud == 0)
> -			return max(min, 9600);
> +			return port->hup_speed;

[Severity: High]
Does this risk a division by zero panic?

If a new struct uart_port is initialized, port->hup_speed defaults to 0.
If a B0 rate is requested before any valid rate is set (such as via
console=ttyS0,0 or an early tcsetattr with B0), this returns 0.

This 0 is then passed down to helpers like uart_get_divisor(), which
calculates the divisor as:

	quot = DIV_ROUND_CLOSEST(port->uartclk, 16 * baud);

It can also reach uart_update_timeout(), which calculates the frame time:

	port->frame_time = (unsigned int)DIV64_U64_ROUND_UP(temp, baud);

In both cases, passing a 0 baud rate will result in a divide-by-zero
hardware exception.

>  
> -		if (baud >= min && baud <= max)
> +		if (baud >= min && baud <= max) {
> +			port->hup_speed = baud;

[Severity: Medium]
Could this result in a data race on port->hup_speed?

In the standard uart_set_termios() path, uart_get_baud_rate() is called
holding only port->mutex and termios_rwsem, not port->lock.

At the same time, atomic console write paths execute asynchronously from
hardirq or NMI holding only the nbcon lock. For example:

drivers/tty/serial/8250/8250_port.c:serial8250_console_restore() {
	...
	baud = serial8250_get_baud_rate(port, &termios, NULL);
	...
}

This calls into uart_get_baud_rate() without holding port->mutex. A
concurrent tcsetattr() and atomic console write can trigger an unprotected
data race on port->hup_speed. A torn read or write on this variable could
yield a garbage baud rate and lead to hardware misconfiguration.

>  			return baud;
> +		}
>  
>  		/*
>  		 * If the range cannot be met then try again with

-- 
Sashiko AI review · https://sashiko.dev/#/patchset/alpine.DEB.2.21.2610120150230.55767@angie.orcam.me.uk?part=1