Re: [PATCH] seccomp: Add group_leader pid to seccomp_notif

On Sun, May 17, 2020 at 09:02:15AM -0600, Tycho Andersen wrote:

On Sun, May 17, 2020 at 08:46:03AM -0600, Tycho Andersen wrote:

quoted

On Sun, May 17, 2020 at 04:33:11PM +0200, Christian Brauner wrote:

quoted

struct seccomp_notif2 {
	__u32 notif_size;
	__u64 id;
	__u32 pid;
	__u32 flags;
	struct seccomp_data data;
	__u32 data_size;
};

I guess you need to put data_size before data, otherwise old userspace
with a smaller struct seccomp_data will look in the wrong place.

But yes, that'll work if you put two sizes in, which is probably
reasonable since we're talking about two structs.

Well, no, it doesn't either. Suppose we add a new field first to
struct seccomp_notif2:

struct seccomp_notif2 {
    __u32 notif_size;
    __u64 id;
    __u32 pid;
    __u32 flags;
    struct seccomp_data data;
    __u32 data_size;
    __u32 new_field;
};

And next we add a new field to struct secccomp_data. When a userspace
compiled with just the new seccomp_notif2 field does:

seccomp_notif2.new_field = ...;

the compiler will put it in the wrong place for the kernel with the
new seccomp_data field too.

Sort of feels like we should do:

struct seccomp_notif2 {
    struct seccomp_notif *notif;
    struct seccomp_data *data;
};

I'm going read this thread more carefully tomorrow, but I just wanted to
mention that I'd *like* to extend seccomp_data for doing deep argument
inspection of the new syscalls. I think it's the least bad of many
designs, and I'll write that up in more detail. (I would *really* like
to avoid extending seccomp's BPF language, and instead allow probing
into the struct copied from userspace, etc.)

Anyway, it's very related to this, so, yeah, probably we need a v2 of the
notif API, but I'll try to get all the ideas here collected in one place.

-- 
Kees Cook