Re: [PATCH v3 2/2] 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 3 potential issue(s) to consider:
- [High] serial: core: out-of-range hup_speed on B0 in uart_get_baud_rate_save() returns 0
- [Medium] serial: dz: dz_set_termios() saves unvalidated baud in hup_speed breaking B0
- [Low] serial: docs: stale uart_get_baud_rate kernel-doc identifier in driver.rst
--

commit 73f85543b84c52391e1ff13d5519c702bc72e894
Author: Maciej W. Rozycki <macro@orcam.me.uk>

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

This commit saves the most recently selected baud rate in port->hup_speed
and uses it instead of max(min, 9600) when hangup is requested via B0. It
also renames uart_get_baud_rate() to uart_get_baud_rate_save() with a save
parameter so serial8250_console_restore() can avoid modifying
port->hup_speed without holding port->mutex.

> diff --git a/drivers/tty/serial/serial_core.c b/drivers/tty/serial/serial_core.c
> index 012f22cf3a60e..18d42491e86d2 100644
> --- a/drivers/tty/serial/serial_core.c
> +++ b/drivers/tty/serial/serial_core.c
> @@ -458,29 +458,32 @@ uart_update_timeout(struct uart_port *port, unsigned int cflag,
>  EXPORT_SYMBOL(uart_update_timeout);
>  
>  /**
> - * uart_get_baud_rate - return baud rate for a particular port
> + * uart_get_baud_rate_save - return baud rate for a particular port

[Severity: Low]
Does Documentation/driver-api/serial/driver.rst also need to be updated for
this rename? It still lists uart_get_baud_rate under the kernel-doc
identifiers for drivers/tty/serial/serial_core.c:

Documentation/driver-api/serial/driver.rst:
.. kernel-doc:: drivers/tty/serial/serial_core.c
   :identifiers: uart_update_timeout uart_get_baud_rate uart_get_divisor

which causes a kernel-doc warning when building the documentation and omits
both uart_get_baud_rate_save() and the new static inline
uart_get_baud_rate() in include/linux/serial_core.h.

[ ... ]

> @@ -519,10 +522,14 @@ uart_get_baud_rate(struct uart_port *port, struct ktermios *termios,
>  		 * Special case: B0 rate.
>  		 */
>  		if (baud == 0)
> -			baud = max(min, 9600);
> +			baud = (port->hup_speed ?
> +				port->hup_speed : max(min, 9600));
>  
> -		if (baud >= min && baud <= max)
> +		if (baud >= min && baud <= max) {
> +			if (save)
> +				port->hup_speed = baud;
>  			return baud;
> +		}

[Severity: High]
What happens in uart_get_baud_rate_save() when B0 is requested and
port->hup_speed is outside the [min, max] range passed by the caller?

Several 8250 drivers dynamically adjust port->uartclk from
tty_termios_baud_rate(termios) before calling serial8250_do_set_termios() or
dw8250_do_set_termios(). For example, in drivers/tty/serial/8250/8250_mid.c:

mid8250_set_termios()
    unsigned int baud = tty_termios_baud_rate(termios);
    ...
    fuart = baud * 16;
    ...
    /* Gracefully handle the B0 case: fall back to B9600 */
    fuart = fuart ? fuart : 9600 * 16;
    ...
    p->uartclk = fuart;
    serial8250_do_set_termios(p, termios, old);

Similar logic exists in byt_set_termios() in
drivers/tty/serial/8250/8250_lpss.c and dw8250_set_termios() in
drivers/tty/serial/8250/8250_dw.c. In addition, port->uartclk or
UPF_MAGIC_MULTIPLIER can be changed via uart_set_info() or
serial8250_update_uartclk().

If a high baud rate (such as 3000000 on Tangier/Penwell in
mid8250_set_termios() or 5000000 on BayTrail in byt_set_termios()) is set first,
that speed is saved in port->hup_speed. When B0 is subsequently requested via
uart_set_termios(), port->uartclk is lowered for the B0 call, reducing max in
serial8250_get_baud_rate() below both port->hup_speed and the speed recorded
in old:

1. On try == 0, baud == 0 is replaced with port->hup_speed, which exceeds max.
   Execution falls through, clears CBAUD in termios, encodes old's high baud
   rate into termios (overwriting B0), sets old = NULL, and continues.
2. On try == 1, baud from old still exceeds max. Because old is now NULL, it
   clips baud to max - 1, encodes that into termios, exits the loop, triggers
   WARN_ON(1), and returns 0.

Returning 0 from uart_get_baud_rate_save() leads to a divide-by-zero in
callers such as serial8250_get_divisor(), dw8250_get_divisor(), and
uart_update_timeout(). Furthermore, because B0 in termios->c_cflag was
overwritten with a non-zero speed, uart_set_termios() in
drivers/tty/serial/serial_core.c no longer sees (cflag & CBAUD) == B0:

uart_set_termios()
    /* Handle transition to B0 status */
    if (((old_termios->c_cflag & CBAUD) != B0) && ((cflag & CBAUD) == B0))
        uart_clear_mctrl(uport, TIOCM_RTS | TIOCM_DTR);

so RTS and DTR are not dropped on hangup.

A related issue occurs in mtk8250_set_termios() in
drivers/tty/serial/8250/8250_mtk.c, which calls serial8250_do_set_termios()
with old == NULL (where max is (port->uartclk + tolerance) / 16) before
calling uart_get_baud_rate() with max = port->uartclk:

mtk8250_set_termios()
    baud = tty_termios_baud_rate(termios);
    serial8250_do_set_termios(port, termios, NULL);
    tty_termios_encode_baud_rate(termios, baud, baud);
    baud = uart_get_baud_rate(port, termios, old,
                              port->uartclk / 16 / UART_DIV_MAX,
                              port->uartclk);

When a high-speed rate above (port->uartclk + tolerance) / 16 is saved in
port->hup_speed and B0 is later requested, the first call inside
serial8250_do_set_termios() sees port->hup_speed > max with old == NULL and
overwrites port->hup_speed with the clipped rate (port->uartclk + tolerance) /
16 - 1 before mtk8250_set_termios() reaches its second uart_get_baud_rate()
call.

Should the B0 fallback clamp port->hup_speed to [min, max] (and avoid saving
back a clamped rate over port->hup_speed when baud == 0)?

[Severity: Medium]
Can saving port->hup_speed inside uart_get_baud_rate_save() also break B0
hangup in drivers/tty/serial/dz.c when an unsupported baud rate within
[50, 9600] is requested?

In dz_set_termios():

drivers/tty/serial/dz.c:dz_set_termios() {
    ...
    baud = uart_get_baud_rate(uport, termios, old_termios, 50, 9600);
    bflag = dz_encode_baud_rate(baud);
    if (bflag < 0) {
        if (old_termios) {
            /* Keep unchanged. */
            baud = tty_termios_baud_rate(old_termios);
            bflag = dz_encode_baud_rate(baud);
        }
        if (bflag < 0) {        /* Resort to 9600.  */
            baud = 9600;
            bflag = DZ_B9600;
        }
        tty_termios_encode_baud_rate(termios, baud, baud);
    }
    ...
}

If userspace requests a baud rate in [50, 9600] that dz_encode_baud_rate()
rejects (such as B200), uart_get_baud_rate() saves 200 into uport->hup_speed
before dz_encode_baud_rate() returns -1 and falls back to old_termios or 9600.

When B0 is later requested to hang up, uart_get_baud_rate() returns
uport->hup_speed (200), dz_encode_baud_rate(200) fails again, and the fallback
block calls tty_termios_encode_baud_rate(termios, baud, baud) with the
non-zero fallback rate. That overwrites B0 in termios->c_cflag, preventing
uart_set_termios() from clearing TIOCM_RTS | TIOCM_DTR.

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