From:   David Laight <David.Laight@ACULAB.COM>
To:     'Jarkko Sonninen' <kasper@iki.fi>
CC:     Johan Hovold <johan@kernel.org>,
        Greg Kroah-Hartman <gregkh@linuxfoundation.org>,
        "linux-usb@vger.kernel.org" <linux-usb@vger.kernel.org>,
        "linux-kernel@vger.kernel.org" <linux-kernel@vger.kernel.org>
Subject: RE: [PATCH v2] USB: serial: xr: Add TIOCGRS485 and TIOCSRS485 ioctls
Thread-Topic: [PATCH v2] USB: serial: xr: Add TIOCGRS485 and TIOCSRS485 ioctls
Thread-Index: AQHZVYXc9R4nUUIaSE2rrq7+pSxNzK75UfkQ
Date:   Mon, 13 Mar 2023 23:02:22 +0000
Message-ID: <f17e5d7c4ccd4db7a5d5001d7dde42da@AcuMS.aculab.com>
References: <ZA7Wh2Z/DdKOsOYr@kroah.com>
 <20230313082734.886890-1-kasper@iki.fi>
In-Reply-To: <20230313082734.886890-1-kasper@iki.fi>
Accept-Language: en-GB, en-US
MIME-Version: 1.0
Content-Language: en-US
Content-Type: text/plain; charset=UTF-8
Content-Transfer-Encoding: 8BIT
Precedence: bulk

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)