linux-kernel - Re: [PATCH] serial: fix TIOCSRS485 locking

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 for Android: free password hash cracker in your pocket

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

Message-ID: <1c814de8-ea36-7a63-34c2-b957d6608cec@linux.intel.com>
Date:   Wed, 12 Apr 2023 16:03:45 +0300 (EEST)
From:   Ilpo Järvinen <ilpo.jarvinen@...ux.intel.com>
To:     Johan Hovold <johan@...nel.org>
cc:     Greg Kroah-Hartman <gregkh@...uxfoundation.org>,
        Jiri Slaby <jirislaby@...nel.org>,
        Arnd Bergmann <arnd@...db.de>, linux-arch@...r.kernel.org,
        linux-serial <linux-serial@...r.kernel.org>,
        LKML <linux-kernel@...r.kernel.org>, stable@...r.kernel.org
Subject: Re: [PATCH] serial: fix TIOCSRS485 locking

On Wed, 12 Apr 2023, Johan Hovold wrote:

> The RS485 multipoint addressing support for some reason added a new
> ADDRB termios cflag which is (only!) updated from one of the RS485
> ioctls.
> 
> Make sure to take the termios rw semaphore for the right ioctl (i.e.
> set, not get).
> 
> Fixes: ae50bb275283 ("serial: take termios_rwsem for ->rs485_config() & pass termios as param")
> Cc: stable@...r.kernel.org	# 6.0
> Cc: Ilpo Järvinen <ilpo.jarvinen@...ux.intel.com>
> Signed-off-by: Johan Hovold <johan@...nel.org>
> ---
> 
> I did not have time to review the multipoint addressing patches at the
> time and only skimmed the archives now, but I can't seem to find any
> motivation for why a precious termios bit was seemingly wasted on ADDRB
> when it is only updated from the RS485 ioctls.
> 
> I hope it wasn't done just to simplify the implementation of
> tty_get_frame_size()? Or was it a left-over from the RFC which
> apparently actually used termios to enable this feature?

No. I made it intentionally. It felt natural place for storing it because 
ADDRB does impact the wire format and cflag is where other wire-format 
impacting bits are also stored.

> Should we consider dropping the Linux-specific ADDRB bit again?
> 
> Johan
> 
> 
>  drivers/tty/serial/serial_core.c | 4 ++--
>  1 file changed, 2 insertions(+), 2 deletions(-)
> 
> diff --git a/drivers/tty/serial/serial_core.c b/drivers/tty/serial/serial_core.c
> index 2bd32c8ece39..728cb72be066 100644
> --- a/drivers/tty/serial/serial_core.c
> +++ b/drivers/tty/serial/serial_core.c
> @@ -1552,7 +1552,7 @@ uart_ioctl(struct tty_struct *tty, unsigned int cmd, unsigned long arg)
>  		goto out;
>  
>  	/* rs485_config requires more locking than others */
> -	if (cmd == TIOCGRS485)
> +	if (cmd == TIOCSRS485)
>  		down_write(&tty->termios_rwsem);
>  
>  	mutex_lock(&port->mutex);
> @@ -1595,7 +1595,7 @@ uart_ioctl(struct tty_struct *tty, unsigned int cmd, unsigned long arg)
>  	}
>  out_up:
>  	mutex_unlock(&port->mutex);
> -	if (cmd == TIOCGRS485)
> +	if (cmd == TIOCSRS485)
>  		up_write(&tty->termios_rwsem);
>  out:
>  	return ret;
> 

Indeed, the caps are so blinding.

Reviewed-by: Ilpo Järvinen <ilpo.jarvinen@...ux.intel.com>

-- 
 i.