linux-kernel - RE: [PATCH] openat2: switch to __attribute__((packed)) for open

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:   Tue, 17 Dec 2019 10:14:48 +0000
From:   David Laight <David.Laight@...LAB.COM>
To:     'Aleksa Sarai' <asarai@...e.de>
CC:     'Aleksa Sarai' <cyphar@...har.com>,
        Rasmus Villemoes <linux@...musvillemoes.dk>,
        Alexander Viro <viro@...iv.linux.org.uk>,
        "Jeff Layton" <jlayton@...nel.org>,
        "J. Bruce Fields" <bfields@...ldses.org>,
        "Shuah Khan" <shuah@...nel.org>,
        "dev@...ncontainers.org" <dev@...ncontainers.org>,
        "containers@...ts.linux-foundation.org" 
        <containers@...ts.linux-foundation.org>,
        "linux-api@...r.kernel.org" <linux-api@...r.kernel.org>,
        "linux-fsdevel@...r.kernel.org" <linux-fsdevel@...r.kernel.org>,
        "linux-kernel@...r.kernel.org" <linux-kernel@...r.kernel.org>,
        "linux-kselftest@...r.kernel.org" <linux-kselftest@...r.kernel.org>
Subject: RE: [PATCH] openat2: switch to __attribute__((packed)) for open_how

>From Aleksa Sarai
> Sent: 17 December 2019 06:47
...
> > Just use u64 for all the fields.
> 
> That is an option (and is the one that clone3 went with), but it's a bit
> awkward because umode_t is a u16 -- and it would be a waste of 6 bytes
> to store it as a u64. Arguably it could be extended but I personally
> find that to be very unlikely (and lots of other syscalls would need be
> updated).

6 bytes on interface structure will make almost no difference.
There is no reason to save more than 16 bits anywhere else.
You could error values with high bits set.

> I'm just going to move the padding to the end and change the error for
> non-zero padding to -E2BIG.

The padding had to be after the u16 field.

> > Use 'flags' bits to indicate whether the additional fields should be looked at.
> > Error if a 'flags' bit requires a value that isn't passed in the structure.
> >
> > Then you can add an extra field and old source code recompiled with the
> > new headers will still work - because the 'junk' value isn't looked at.
> 
> This problem is already handled entirely by copy_struct_from_user().
> 
> It is true that for some new fields it will be necessary to add a new
> flag (such as passing fds -- where 0 is a valid value) but for most new
> fields (especially pointer or flag fields) it will not be necessary
> because the 0 value is equivalent to the old behaviour. It also allows
> us to entirely avoid accepting junk from userspace.

Only if userspace is guaranteed to memset the entire structure
before making the call - rather than just fill in all the fields it knows about.
If it doesn't use memset() then recompiling old code with new headers
will pass garbage to the kernel.
copy_struct_from_user() cannot solve that problem.
You'll never be able to guarantee that all code actually clears the
entire structure - so at some point extending it will break recompilations
of old code - annoying.

	David

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