On Tue, Oct 30, 2018 at 5:29 PM, Tycho Andersen [off-list ref] wrote:

On Tue, Oct 30, 2018 at 03:34:54PM -0700, Kees Cook wrote:

quoted

On Tue, Oct 30, 2018 at 3:32 PM, Tycho Andersen [off-list ref] wrote:

quoted

On Tue, Oct 30, 2018 at 03:00:17PM -0700, Kees Cook wrote:

quoted

On Tue, Oct 30, 2018 at 2:54 PM, Tycho Andersen [off-list ref] wrote:

quoted

On Tue, Oct 30, 2018 at 02:49:21PM -0700, Kees Cook wrote:

quoted

On Mon, Oct 29, 2018 at 3:40 PM, Tycho Andersen [off-list ref] wrote:

quoted

    * switch to a flags based future-proofing mechanism for struct
      seccomp_notif and seccomp_notif_resp, thus avoiding version issues
      with structure length (Kees)

[...]

quoted

+struct seccomp_notif {
+       __u64 id;
+       __u32 pid;
+       __u32 flags;
+       struct seccomp_data data;
+};
+
+struct seccomp_notif_resp {
+       __u64 id;
+       __s64 val;
+       __s32 error;
+       __u32 flags;
+};

Hrm, so, what's the plan for when struct seccomp_data changes size?

I guess my plan was don't ever change the size again, just use flags
and have extra state available via ioctl().

quoted

I'm realizing that it might be "too late" for userspace to discover
it's running on a newer kernel. i.e. it gets a user notification, and
discovers flags it doesn't know how to handle. Do we actually need
both flags AND a length? Designing UAPI is frustrating! :)

:). I don't see this as such a big problem -- in fact it's better than
the length mode, where you don't know what you don't know, because it
only copied as much info as you could handle. Older userspace would
simply not use information it didn't know how to use.

quoted

Do we need another ioctl to discover the seccomp_data size maybe?

That could be an option as well, assuming we agree that size would
work, which I thought we didn't?

Size alone wasn't able to determine the layout of the seccomp_notif
structure since it had holes (in the prior version). seccomp_data
doesn't have holes and is likely to change in size (see the recent
thread on adding the MPK register to it...)

Oh, sorry, I misread this as seccomp_notif, not seccomp_data.

quoted

I'm trying to imagine the right API for this. A portable user of
seccomp_notif expects the id/pid/flags/data to always be in the same
place, but it's the size of seccomp_data that may change. So it wants
to allocate space for seccomp_notif header and "everything else", of
which is may only understand the start of seccomp_data (and ignore any
new trailing fields).

So... perhaps the "how big are things?" ioctl would report the header
size and the seccomp_data size. Then both are flexible. And flags
would be left as a way to "version" the header?

Any Linux API list members want to chime in here?

So:

struct seccomp_notify_sizes {
    u16 seccomp_notify;
    u16 seccomp_data;
};

ioctl(fd, SECCOMP_IOCTL_GET_SIZE, &sizes);

This would be only one extra syscall over the lifetime of the listener
process, which doesn't seem too bad. One thing that's slightly
annoying is that you can't do it until you actually get an event, so
maybe it could be a command on the seccomp syscall instead:

seccomp(SECCOMP_GET_NOTIF_SIZES, 0, &sizes);

Yeah, top-level makes more sense. u16 seems fine too.

So one problem is this is that the third argument of the seccomp
syscall is declared as const char, so I get:

kernel/seccomp.c: In function ‘seccomp_get_notif_sizes’:
kernel/seccomp.c:1401:19: warning: passing argument 1 of ‘copy_to_user’ discards ‘const’ qualifier from pointer target type [-Wdiscarded-qualifiers]
  if (copy_to_user(usizes, &sizes, sizeof(sizes)))
                   ^~~~~~
In file included from ./include/linux/compat.h:19:0,
                 from kernel/seccomp.c:19:
./include/linux/uaccess.h:152:1: note: expected ‘void *’ but argument is of type ‘const char *’
 copy_to_user(void __user *to, const void *from, unsigned long n)
 ^~~~~~~~~~~~

If I drop the const it doesn't complain, but I'm not sure what the protocol is
for changing the types of syscall declarations. In principle it doesn't really
mean anything, but...

I think this should be fine. It's documented as "void *"...

-- 
Kees Cook