linux-kernel - RE: [PATCH v2] USB: serial: xr: Add TIOCGRS485 and TIOCSRS485 ioctls

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]

Message-ID: <f17e5d7c4ccd4db7a5d5001d7dde42da@AcuMS.aculab.com>
Date:   Mon, 13 Mar 2023 23:02:22 +0000
From:   David Laight <David.Laight@...LAB.COM>
To:     'Jarkko Sonninen' <kasper@....fi>
CC:     Johan Hovold <johan@...nel.org>,
        Greg Kroah-Hartman <gregkh@...uxfoundation.org>,
        "linux-usb@...r.kernel.org" <linux-usb@...r.kernel.org>,
        "linux-kernel@...r.kernel.org" <linux-kernel@...r.kernel.org>
Subject: RE: [PATCH v2] USB: serial: xr: Add TIOCGRS485 and TIOCSRS485 ioctls

From: Jarkko Sonninen
> Sent: 13 March 2023 08:28
> 
> Add support for RS-485 in Exar USB adapters.
> RS-485 mode is controlled by TIOCGRS485 and TIOCSRS485 ioctls.
> Gpio mode register is set to enable RS-485.

The locking is entirely dubious.
Summary:

Taking the lock to read the flags is pretty pointless.
You are only looking at one bit and nothing else is tied
to the lock.
Even a READ_ONCE() isn't needed.
> +	spin_lock_irqsave(&data->lock, flags);
> +	rs485_flags = data->rs485.flags;
> +	spin_unlock_irqrestore(&data->lock, flags);
> +	if (rs485_flags & SER_RS485_ENABLED)
> +		gpio_mode |= XR_GPIO_MODE_SEL_RS485 | XR_GPIO_MODE_RS485_TX_H;
> +	else if (C_CRTSCTS(tty) && C_BAUD(tty) != B0)
> +		gpio_mode |= XR_GPIO_MODE_SEL_RTS_CTS;
> +

The ioctl read code reads the data unlocked.
> +	if (copy_to_user(argp, &data->rs485, sizeof(data->rs485)))
> +		return -EFAULT;
So could return old and new data if the ioctl write code
runs concurrently (and you get a hardware interrupt or page
fault mid-buffer).

The ioctl write code acquires the lock across a structure copy.
(which should be a structure copy, not a memcpy).
The only way the lock will have any effect is if multiple
threads are doing updates at the same time.
Code doing that won't work anyway.
> +	if (copy_from_user(&rs485, argp, sizeof(rs485)))
> +		return -EFAULT;
> +
> +	dev_dbg(tty->dev, "Flags %02x\n", rs485.flags);
> +	rs485.flags &= SER_RS485_ENABLED;
> +	spin_lock_irqsave(&data->lock, flags);
> +	memcpy(&data->rs485, &rs485, sizeof(rs485));
> +	spin_unlock_irqrestore(&data->lock, flags);

In any case you one seem to be implementing one bit of
the flags - so the rest of the data can be ignored.

	David

-
Registered Address Lakeside, Bramley Road, Mount Farm, Milton Keynes, MK1 1PT, UK
Registration No: 1397386 (Wales)