linux-kernel - Re: [PATCH v6 1/7] serial: Do not hold the port lock when setting rx-during-tx GPIO

lists.openwall.net		lists / announce owl-users owl-dev john-users john-dev passwdqc-users yescrypt popa3d-users / oss-security kernel-hardening musl sabotage tlsify passwords / crypt-dev xvendor / Bugtraq Full-Disclosure linux-kernel linux-netdev linux-ext4 linux-hardening linux-cve-announce PHC
Open Source and information security mailing list archives

Hash Suite: Windows password security audit tool. GUI, reports in PDF.

[<prev] [next>] [<thread-prev] [thread-next>] [day] [month] [year] [list]

Date: Mon, 25 Dec 2023 13:31:16 +0100
From: Maarten Brock <m.brock@...mierlo.com>
To: Lino Sanfilippo <l.sanfilippo@...bus.com>
Cc: gregkh@...uxfoundation.org, jirislaby@...nel.org,
 ilpo.jarvinen@...ux.intel.com, u.kleine-koenig@...gutronix.de,
 shawnguo@...nel.org, s.hauer@...gutronix.de, mcoquelin.stm32@...il.com,
 alexandre.torgue@...s.st.com, cniedermaier@...electronics.com,
 hugo@...ovil.com, linux-kernel@...r.kernel.org,
 linux-serial@...r.kernel.org, LinoSanfilippo@....de, lukas@...ner.de,
 p.rosenberger@...bus.com, stable@...r.kernel.org, Hugo Villeneuve
 <hvilleneuve@...onoff.com>
Subject: Re: [PATCH v6 1/7] serial: Do not hold the port lock when setting
 rx-during-tx GPIO

Lino Sanfilippo wrote on 2023-12-25 12:35:
> diff --git a/drivers/tty/serial/serial_core.c 
> b/drivers/tty/serial/serial_core.c
> index f1348a509552..d155131f221d 100644
> --- a/drivers/tty/serial/serial_core.c
> +++ b/drivers/tty/serial/serial_core.c
> @@ -1402,6 +1402,16 @@ static void uart_set_rs485_termination(struct
> uart_port *port,
>  				 !!(rs485->flags & SER_RS485_TERMINATE_BUS));
>  }
> 
> +static void uart_set_rs485_rx_during_tx(struct uart_port *port,
> +					const struct serial_rs485 *rs485)
> +{
> +	if (!(rs485->flags & SER_RS485_ENABLED))
> +		return;
> +

How about checking port->rs485_rx_during_tx_gpio here against NULL 
instead of
before every call?

> +	gpiod_set_value_cansleep(port->rs485_rx_during_tx_gpio,
> +				 !!(rs485->flags & SER_RS485_RX_DURING_TX));
> +}
> +
>  static int uart_rs485_config(struct uart_port *port)
>  {
>  	struct serial_rs485 *rs485 = &port->rs485;
> @@ -1413,12 +1423,17 @@ static int uart_rs485_config(struct uart_port 
> *port)
> 
>  	uart_sanitize_serial_rs485(port, rs485);
>  	uart_set_rs485_termination(port, rs485);
> +	uart_set_rs485_rx_during_tx(port, rs485);
> 
>  	uart_port_lock_irqsave(port, &flags);
>  	ret = port->rs485_config(port, NULL, rs485);
>  	uart_port_unlock_irqrestore(port, flags);
> -	if (ret)
> +	if (ret) {
>  		memset(rs485, 0, sizeof(*rs485));
> +		/* unset GPIOs */
> +		gpiod_set_value_cansleep(port->rs485_term_gpio, 0);
> +		gpiod_set_value_cansleep(port->rs485_rx_during_tx_gpio, 0);
> +	}
> 
>  	return ret;
>  }
> @@ -1457,6 +1472,7 @@ static int uart_set_rs485_config(struct
> tty_struct *tty, struct uart_port *port,
>  		return ret;
>  	uart_sanitize_serial_rs485(port, &rs485);
>  	uart_set_rs485_termination(port, &rs485);
> +	uart_set_rs485_rx_during_tx(port, &rs485);
> 
>  	uart_port_lock_irqsave(port, &flags);
>  	ret = port->rs485_config(port, &tty->termios, &rs485);
> @@ -1468,8 +1484,14 @@ static int uart_set_rs485_config(struct
> tty_struct *tty, struct uart_port *port,
>  			port->ops->set_mctrl(port, port->mctrl);
>  	}
>  	uart_port_unlock_irqrestore(port, flags);
> -	if (ret)
> +	if (ret) {
> +		/* restore old GPIO settings */
> +		gpiod_set_value_cansleep(port->rs485_term_gpio,
> +			!!(port->rs485.flags & SER_RS485_TERMINATE_BUS));
> +		gpiod_set_value_cansleep(port->rs485_rx_during_tx_gpio,
> +			!!(port->rs485.flags & SER_RS485_RX_DURING_TX));

This does not look like restoring.
Further this looks suspiciously like duplicated code.

>  		return ret;
> +	}
> 
>  	if (copy_to_user(rs485_user, &port->rs485, sizeof(port->rs485)))
>  		return -EFAULT;
> diff --git a/drivers/tty/serial/stm32-usart.c 
> b/drivers/tty/serial/stm32-usart.c
> index 3048620315d6..ec9a72a5bea9 100644
> --- a/drivers/tty/serial/stm32-usart.c
> +++ b/drivers/tty/serial/stm32-usart.c
> @@ -226,10 +226,7 @@ static int stm32_usart_config_rs485(struct
> uart_port *port, struct ktermios *ter
> 
>  	stm32_usart_clr_bits(port, ofs->cr1, BIT(cfg->uart_enable_bit));
> 
> -	if (port->rs485_rx_during_tx_gpio)
> -		gpiod_set_value_cansleep(port->rs485_rx_during_tx_gpio,
> -					 !!(rs485conf->flags & SER_RS485_RX_DURING_TX));
> -	else
> +	if (!port->rs485_rx_during_tx_gpio)

Should the ! be there?

>  		rs485conf->flags |= SER_RS485_RX_DURING_TX;
> 
>  	if (rs485conf->flags & SER_RS485_ENABLED) {

Kind Regards
Maarten Brock