On Wed, Jul 08, 2020 at 11:12:02PM -0700, Kees Cook wrote:
On Tue, Jul 07, 2020 at 03:30:49PM +0200, Christian Brauner wrote:
Hm, maybe change that description to sm like:
[...]
Cool, yeah. Thanks! I've tweaked it a little more
- /* 24 is original sizeof(struct seccomp_notif_addfd) */
- if (size < 24 || size >= PAGE_SIZE)
return -EINVAL;Hm, so maybe add the following:
#define SECCOMP_NOTIFY_ADDFD_VER0 24 #define SECCOMP_NOTIFY_ADDFD_LATEST SECCOMP_NOTIFY_ADDFD_VER0
and then place:
BUILD_BUG_ON(sizeof(struct seccomp_notify_addfd) < SECCOMP_NOTIFY_ADDFD_VER0); BUILD_BUG_ON(sizeof(struct open_how) != SECCOMP_NOTIFY_ADDFD_LATEST);
Yes, good idea (BTW, did the EA syscall docs land?)
I'll be giving a kernel summit talk about extensible syscalls to come to some agreement on a few things. After this we'll update the doc patch we have now and merge it. :)
I've made these SECCOMP_NOTIFY_ADDFD_SIZE_* to match your examples below (i.e. I added "SIZE" to what you suggested above).
Yup, sounds good!
somewhere which is what we do for clone3(), openat2() and others to catch build-time nonsense.
include/uapi/linux/perf_event.h:#define PERF_ATTR_SIZE_VER0 64 /* sizeof first published struct */ include/uapi/linux/sched.h:#define CLONE_ARGS_SIZE_VER0 64 /* sizeof first published struct */ include/uapi/linux/sched/types.h:#define SCHED_ATTR_SIZE_VER0 48 /* sizeof first published struct */ include/linux/fcntl.h:#define OPEN_HOW_SIZE_VER0 24 /* sizeof first published struct */ include/linux/fcntl.h:#define OPEN_HOW_SIZE_LATEST OPEN_HOW_SIZE_VER0
The ..._SIZE_VER0 and ...LATEST stuff doesn't seem useful to export via UAPI. Above, 2 of the 3 export to uapi. Is there a specific rationale for which should and which shouldn't?
I think openat2() just didn't think it was useful. I find them helpful because I often update codebase to the newest struct I know about:
struct clone_args { __aligned_u64 flags; __aligned_u64 pidfd; __aligned_u64 child_tid; __aligned_u64 parent_tid; __aligned_u64 exit_signal; __aligned_u64 stack; __aligned_u64 stack_size; __aligned_u64 tls; /* CLONE_ARGS_SIZE_VER0 64 */ __aligned_u64 set_tid; __aligned_u64 set_tid_size; /* CLONE_ARGS_SIZE_VER1 80 */ __aligned_u64 cgroup; /* CLONE_ARGS_SIZE_VER2 88 */ };
But bumping it means I can't use:
clone3(&clone_args, sizeof(clone));
everywhere in the codebase because I'm fscking over everyone on older kernels now. :)
Soin various parts of the codebase I will just use:
clone3(&clone_args, CLONE_ARGS_SIZE_VER0);
because I don't care about any of the additional features and I don't need the kernel to copy any of the other stuff. Then in other parts of the codebase I want to set_tid so I use:
clone3(&clone_args, CLONE_ARGS_SIZE_VER1);
This way I can also set "templates", i.e.
struct clone_args clone_template1 = { .flags |= CLONE_CLEAR_SIGHAND, .exit_signal = SIGCHLD, .set_tid = 1000, .set_tid_size = 1, };
and then use the same struct for:
clone3(&clone_template1, CLONE_ARGS_SIZE_VER0); clone3(&clone_template1, CLONE_ARGS_SIZE_VER1);
Whereas sizeof(clone_template1) would always give me CLONE_ARGS_SIZE_VER2.
Christian