linux-kernel - Re: RFC: out-of-tree tty driver breakage (changing ASYNC

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:	Sun, 10 Jan 2016 23:44:39 +0000
From:	One Thousand Gnomes <gnomes@...rguk.ukuu.org.uk>
To:	Peter Hurley <peter@...leysoftware.com>
Cc:	"linux-serial@...r.kernel.org" <linux-serial@...r.kernel.org>,
	Linux kernel <linux-kernel@...r.kernel.org>,
	Greg KH <gregkh@...uxfoundation.org>,
	Jiri Slaby <jslaby@...e.cz>
Subject: Re: RFC: out-of-tree tty driver breakage (changing ASYNC_ bits)

On Sun, 10 Jan 2016 13:42:44 -0800
Peter Hurley <peter@...leysoftware.com> wrote:

> The tty/serial core uses 5 bits in the tty_port.flags field to
> manage state. They are:
> 
> ASYNCB_INITIALIZED
> ASYNCB_SUSPENDED
> ASYNCB_NORMAL_ACTIVE
> - and -
> ASYNCB_CTS_FLOW
> ASYNCB_CHECK_CD
> 
> Unfortunately, updates to this field (tty_port.flags) are often
> non-atomic. Additionally, the field is visible to/modifiable by
> userspace (the first 3 bits above are not modifiable by userspace
> though). Lastly, the multi-bit ASYNC_SPD_ bitfield is in this
> tty_port.flags field as well.

ASYNC_SPD ought to just get retired, it's been obsolete and warning
people since forever 8)

Two comments:

1. Making something unsigned long doesn't magically make it atomic. You
either use atomic_foo() or you use set_bit() and friends or the compiler
sneaks up on you and does evil things. It might make it "a bit more
atomic" but it doesn't make it correct. The compiler is free to do stupid
things like turn

                x |= 1

into
		store 1 to memory
		or memory with reg (holding old x)

gcc won't afaik ever do that on any platform we support, but it's not
against the rules if its ever optimal !

2. On a lot of architectures it's going to be easier to just use set_bit()
and friends I suspect than take the cache hit of 5 unsigned longs. At the
very least re-order the struct to keep the hot stuff together.

The compiler will also play other games with your intentions. It'll defer
or even eliminate invisible writes that don't get protected by memory
barriers or forced by say function calls.

Alan