Re: [PATCH v2 3/3] serial: 8250_mxpcie: take the line settings from the new termios

[Jiri Slaby <jirislaby@kernel.org>]

CC Crescent as well.

On 01. 09. 26, 6:23, sashiko-bot@kernel.org wrote:
> Thank you for your contribution! Sashiko AI review found 1 potential issue(s) to consider:
> 
> Pre-existing issues:
> - [High] Hardware registers and port status are modified without holding the port lock or a Runtime PM reference.
> --
> 
> commit cc8d50f2957c7d540f5a4840cf6ed31ccd1bed47
> Author: Linmao Li <lilinmao@kylinos.cn>
> 
> serial: 8250_mxpcie: take the line settings from the new termios
> 
> mxpcie8250_set_termios() reads the line settings out of port->state->port.tty,
> which is only set once the port has been opened. This commit fixes a NULL
> dereference during console setup or kgdboc attach by reading the settings
> from the termios the serial core passes in instead.
> 
>> diff --git a/drivers/tty/serial/8250/8250_mxpcie.c b/drivers/tty/serial/8250/8250_mxpcie.c
>> index ef2516ec16da0..19f1f51eec477 100644
>> --- a/drivers/tty/serial/8250/8250_mxpcie.c
>> +++ b/drivers/tty/serial/8250/8250_mxpcie.c
>> @@ -218,8 +218,6 @@ static void mxpcie8250_set_termios(struct uart_port *port,
>>   				   const struct ktermios *old)
>>   {
>>   	struct uart_8250_port *up = up_to_u8250p(port);
>> -	struct tty_struct *tty = port->state->port.tty;
>> -	unsigned int cflag = tty->termios.c_cflag;
>>   	u8 efr, val;
>>   
>>   	serial8250_do_set_termios(port, new, old);
>> @@ -229,23 +227,25 @@ static void mxpcie8250_set_termios(struct uart_port *port,
>>   	efr = serial_in(up, MOXA_PUART_EFR);
> 
> [Severity: High]
> This is a pre-existing issue, but is it safe to modify the port status and
> access hardware registers here after serial8250_do_set_termios() has
> already released the port lock and runtime PM reference?
> 
> Modifying up->port.status in mxpcie8250_set_termios() without holding the
> port lock creates a data race that can corrupt flow control flags
> (e.g., UPSTAT_AUTOCTS). This directly violates the locking contract
> documented in include/linux/serial_core.h.
> 
> Furthermore, accessing UART hardware registers via serial_in() and
> serial_out() without a port lock or a Runtime PM reference can lead to
> concurrent register access races with IRQ handlers, or bus faults if the
> device enters a suspended state. This sequence occurs every time
> mxpcie8250_set_termios() is called, for instance when userspace calls
> tcsetattr to change flow control on the TTY device.
> 
>>   	efr &= ~(MOXA_PUART_EFR_AUTO_RTS | MOXA_PUART_EFR_AUTO_CTS);
>>   
>> -	if (cflag & CRTSCTS) {
>> +	if (new->c_cflag & CRTSCTS) {
>>   		efr |= (MOXA_PUART_EFR_AUTO_RTS | MOXA_PUART_EFR_AUTO_CTS);
>>   		up->port.status |= (UPSTAT_AUTORTS | UPSTAT_AUTOCTS);
>>   	}
> 


-- 
js
suse labs