Re: [PATCH] seccomp: Check flags on seccomp_notif is unset

On December 27, 2019 4:15:01 PM GMT+01:00, Aleksa Sarai [off-list ref] wrote:

On 2019-12-27, Sargun Dhillon [off-list ref] wrote:

quoted

On Fri, Dec 27, 2019 at 6:47 AM Christian Brauner
[off-list ref] wrote:

quoted

On Fri, Dec 27, 2019 at 01:31:31PM +1100, Aleksa Sarai wrote:

quoted

On 2019-12-27, Aleksa Sarai [off-list ref] wrote:

Scratch that -- as Tycho just mentioned, there is un-named

padding in

quoted

quoted

quoted

the struct so check_zeroed_user() is the wrong thing to do. But

this

quoted

quoted

Hm, I don't think so.
I understood Tycho's point as _if_ there ever is padding then this

would

quoted

quoted

not be zeroed.
Right now, there is no padding since the struct is correctly

padded:

quoted

quoted

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 */

quoted

quoted

        __u32                      arch;                 /*     4  

 4 */

quoted

quoted

        __u64                      instruction_pointer;  /*     8  

 8 */

quoted

quoted

        __u64                      args[6];              /*    16  

48 */

quoted

quoted

        /* size: 64, cachelines: 1, members: 4 */
};
struct seccomp_notif {
        __u64                      id;                   /*     0  

 8 */

quoted

quoted

        __u32                      pid;                  /*     8  

 4 */

quoted

quoted

        __u32                      flags;                /*    12  

 4 */

quoted

quoted

        struct seccomp_data data;                        /*    16  

64 */

quoted

quoted

        /* size: 80, cachelines: 2, members: 4 */
        /* last cacheline: 16 bytes */
};

The only worry would be a 2byte int type but there's no

architecture

quoted

quoted

we support which does this right now afaict.

quoted

also will make extensions harder to deal with because

(presumably) they

quoted

quoted

quoted

will also have un-named padding, making copy_struct_from_user()

the

quoted

quoted

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

quoted

quoted

that _all_ fields are uninitialized and not just the flags argument
since we might introduce a flags argument that requires another

already

quoted

quoted

existing member in seccomp_notif to be set to a value. We should do

this

quoted

quoted

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

quoted

quoted

and they will likely need some extensions. That includes the pidfd

stuff

quoted

quoted

for seccomp that Sargun will likely be doing and the new

pidfd_getfd()

quoted

quoted

syscall. So it's not unlikely that we might need other already

existing

quoted

quoted

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.

quoted

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

quoted

must be manually zeroed out. Although, this might be laying a trap

for

quoted

ourselves.

This leaves us in a good position for introducing a flag field in the

future.

quoted

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