Messages in this thread | | | Date | Fri, 12 Jun 2020 08:13:25 -0700 | From | Kees Cook <> | Subject | Re: [PATCH v3 1/4] fs, net: Standardize on file_receive helper to move fds across processes |
| |
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! ;)
-- Kees Cook
| |