linux-kernel - Re: [PATCH] seccomp: Check flags on seccomp

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] [day] [month] [year] [list]

Message-ID: <321F2BE8-6F16-4804-9F20-B03E5800B940@ubuntu.com>
Date:   Fri, 27 Dec 2019 16:32:46 +0100
From:   Christian Brauner <christian.brauner@...ntu.com>
To:     Aleksa Sarai <cyphar@...har.com>, Sargun Dhillon <sargun@...gun.me>
CC:     LKML <linux-kernel@...r.kernel.org>,
        Linux API <linux-api@...r.kernel.org>,
        Tycho Andersen <tycho@...ho.ws>, Jann Horn <jannh@...gle.com>,
        Kees Cook <keescook@...omium.org>
Subject: Re: [PATCH] seccomp: Check flags on seccomp_notif is unset

On December 27, 2019 4:15:01 PM GMT+01:00, Aleksa Sarai <cyphar@...har.com> wrote:
>On 2019-12-27, Sargun Dhillon <sargun@...gun.me> wrote:
>> On Fri, Dec 27, 2019 at 6:47 AM Christian Brauner
>> <christian.brauner@...ntu.com> wrote:
>> >
>> > On Fri, Dec 27, 2019 at 01:31:31PM +1100, Aleksa Sarai wrote:
>> > > On 2019-12-27, Aleksa Sarai <cyphar@...har.com> wrote:
>> > >
>> > > Scratch that -- as Tycho just mentioned, there is un-named
>padding in
>> > > the struct so check_zeroed_user() is the wrong thing to do. But
>this
>> >
>> > Hm, I don't think so.
>> > I understood Tycho's point as _if_ there ever is padding then this
>would
>> > not be zeroed.
>> > Right now, there is no padding since the struct is correctly
>padded:
>> >
>> > struct seccomp_data {
>> >         int nr;
>> >         __u32 arch;
>> >         __u64 instruction_pointer;
>> >         __u64 args[6];
>> > };
>> >
>> > struct seccomp_notif {
>> >         __u64 id;
>> >         __u32 pid;
>> >         __u32 flags;
>> >         struct seccomp_data data;
>> > };
>> >
>> > which would be - using pahole:
>> >
>> > struct seccomp_data {
>> >         int                        nr;                   /*     0  
>  4 */
>> >         __u32                      arch;                 /*     4  
>  4 */
>> >         __u64                      instruction_pointer;  /*     8  
>  8 */
>> >         __u64                      args[6];              /*    16  
> 48 */
>> >
>> >         /* size: 64, cachelines: 1, members: 4 */
>> > };
>> > struct seccomp_notif {
>> >         __u64                      id;                   /*     0  
>  8 */
>> >         __u32                      pid;                  /*     8  
>  4 */
>> >         __u32                      flags;                /*    12  
>  4 */
>> >         struct seccomp_data data;                        /*    16  
> 64 */
>> >
>> >         /* size: 80, cachelines: 2, members: 4 */
>> >         /* last cacheline: 16 bytes */
>> > };
>> >
>> > The only worry would be a 2byte int type but there's no
>architecture
>> > we support which does this right now afaict.
>> >
>> > > also will make extensions harder to deal with because
>(presumably) they
>> > > will also have un-named padding, making copy_struct_from_user()
>the
>> >
>> > This all will be a non-issue if we just use __u64 for extensions.
>> >
>> > My point about using copy_struct_from_user() was that we should
>verify
>> > that _all_ fields are uninitialized and not just the flags argument
>> > since we might introduce a flags argument that requires another
>already
>> > existing member in seccomp_notif to be set to a value. We should do
>this
>> > change now so we don't have to risk breaking someone in the future.
>> >
>> > I'm trying to get at least Mozilla/Firefox off of their crazy
>> > SECCOMP_RET_TRAP way of implementing their broker onto the user
>notifier
>> > and they will likely need some extensions. That includes the pidfd
>stuff
>> > for seccomp that Sargun will likely be doing and the new
>pidfd_getfd()
>> > syscall. So it's not unlikely that we might need other already
>existing
>> > fields in that struct to be set to some value.
>> >
>> > I don't particulary care how we do it:
>> > - We can do a simple copy_from_user() and check each field
>individually.
>> 
>> Just doing a simple copy_from_user, and for now, calling memchr_inv
>> on the whole thing. We can drop the memset, and just leave a note to
>> indicate that if unpadded fields are introduced in the future, this
>structure
>> must be manually zeroed out. Although, this might be laying a trap
>for
>> ourselves.
>> 
>> This leaves us in a good position for introducing a flag field in the
>future.
>> All we have to do is change the memchr_inv from checking on an
>> entire struct basis to checking on a per-field basis.
>
>There is no need to do memchr_inv() on copy_from_user() to check for
>zero-ness. That's the entire point of check_zeroed_user() -- to not
>need
>to do it that way.

Right, we added that too a while ago.
Let's use it.

Christian