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;
};

?

Oh yes of course, sorry that was my stupid typo. I meant:

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

at which point things should just work imho. This is similar to how the
set_tid array works. The kernel doesn't need to allocate any more too.
The kernel can just always use the currently know seccomp_data size.
If the kernel supports _less_ than what the caller expects, it can
report the supported size in data_size to userspace returning EINVAL. If
it supports more then it can just copy the known fields, I guess.

This way we don't need to add yet another ioctl...

Christian