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?)
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.
default: return -EINVAL; } }
What boxes does this tick?
- Forwards (and backwards) compatibility
- Applies to existing commands
- Command can be extended without requiring new ioctl to be defined
(Technically, a new one is always redefined, but it's automatic in that the kernel needs to do nothing.)
- It well accomodates the future where we want to have a kernel helper copy the structures from userspace
Yeah, this is a good solution.
The fact that the size of the argument struct, and the ioctl are defined in the same header gives us the ability to "cheat", and for the argument size to be included / embedded for free in the command passed to ioctl. In turn, this gives us two benefits. First, it means we don't have to copy from user twice, and can just do it all in one shot since the size is passed with the syscall arguments. Second, it means that the user does not have to do the following:
seccomp_notif_addfd addfd = {}; addfd.size = sizeof(struct seccomp_notif_addfd)
Because sizeof(struct seccomp_notif_addfd) is embedded in SECCOMP_IOCTL_NOTIF_ADDFD based on the same headers they plucked the struct out of.
Cool. I will do more patch reworking! ;)