On Fri, Jun 12, 2020 at 08:13:25AM -0700, Kees Cook wrote:
On Fri, Jun 12, 2020 at 10:46:30AM +0000, Sargun Dhillon wrote:
My suggest, written out (no idea if this code actually works), is as follows:
ioctl.h: /* This needs to be added */ #define IOCDIR_MASK (_IOC_DIRMASK << _IOC_DIRSHIFT)
This exists already:
#define _IOC_DIRMASK ((1 << _IOC_DIRBITS)-1)
seccomp.h:
struct struct seccomp_notif_addfd { __u64 fd; ... }
/* or IOW? */ #define SECCOMP_IOCTL_NOTIF_ADDFD SECCOMP_IOWR(3, struct seccomp_notif_addfd)
seccomp.c: static long seccomp_notify_addfd(struct seccomp_filter *filter, struct seccomp_notif_addfd __user *uaddfd int size) { struct seccomp_notif_addfd addfd; int ret;
if (size < 32) return -EINVAL; if (size > PAGE_SIZE) return -E2BIG;
(Tanget: what was the reason for copy_struct_from_user() not including the min/max check? I have a memory of Al objecting to having an "internal" limit?)
Al didn't want the PAGE_SIZE limit in there because there's nothing inherently wrong with copying insane amounts of memory.
(Another tangent. I've asked this on Twitter not too long ago: do we have stats how long copy_from_user()/copy_struct_from_user() takes with growing struct/memory size? I'd be really interested in this. I have a feeling that clone3()'s and - having had a chat with David Howells - openat2()'s structs will continue to grow for a while... and I'd really like to have some numbers on when copy_struct_from_user() becomes costly or how costly it becomes.)
ret = copy_struct_from_user(&addfd, sizeof(addfd), uaddfd, size); if (ret) return ret;
... }
/* Mask out size */ #define SIZE_MASK(cmd) (~IOCSIZE_MASK & cmd)
/* Mask out direction */ #define DIR_MASK(cmd) (~IOCDIR_MASK & cmd)
static long seccomp_notify_ioctl(struct file *file, unsigned int cmd, unsigned long arg) { struct seccomp_filter *filter = file->private_data; void __user *buf = (void __user *)arg;
/* Fixed size ioctls. Can be converted later on? */ switch (cmd) { case SECCOMP_IOCTL_NOTIF_RECV: return seccomp_notify_recv(filter, buf); case SECCOMP_IOCTL_NOTIF_SEND: return seccomp_notify_send(filter, buf); case SECCOMP_IOCTL_NOTIF_ID_VALID: return seccomp_notify_id_valid(filter, buf); }
/* Probably should make some nicer macros here */ switch (SIZE_MASK(DIR_MASK(cmd))) { case SIZE_MASK(DIR_MASK(SECCOMP_IOCTL_NOTIF_ADDFD)):
Ah yeah, I like this because of what you mention below: it's forward compat too. (I'd just use the ioctl masks directly...)
switch (cmd & ~(_IOC_SIZEMASK | _IOC_DIRMASK))
return seccomp_notify_addfd(filter, buf, _IOC_SIZE(cmd));I really like that this ends up having the same construction as a standard EA syscall: the size is part of the syscall arguments.
This is basically what I had proposed in my previous mail, right?
Christian