linux-kernel - Re: [PATCH v3 1/4] fs, net: Standardize on file_receive helper to move fds across processes

lists.openwall.net		lists / announce owl-users owl-dev john-users john-dev passwdqc-users yescrypt popa3d-users / oss-security kernel-hardening musl sabotage tlsify passwords / crypt-dev xvendor / Bugtraq Full-Disclosure linux-kernel linux-netdev linux-ext4 linux-hardening linux-cve-announce PHC
Open Source and information security mailing list archives

Hash Suite: Windows password security audit tool. GUI, reports in PDF.

[<prev] [next>] [<thread-prev] [thread-next>] [day] [month] [year] [list]

Message-ID: <20200612182816.okwylihs6u6wkgxd@wittgenstein>
Date:   Fri, 12 Jun 2020 20:28:16 +0200
From:   Christian Brauner <christian.brauner@...ntu.com>
To:     Kees Cook <keescook@...omium.org>
Cc:     Sargun Dhillon <sargun@...gun.me>,
        Giuseppe Scrivano <gscrivan@...hat.com>,
        Robert Sesek <rsesek@...gle.com>,
        Chris Palmer <palmer@...gle.com>, Jann Horn <jannh@...gle.com>,
        Greg Kroah-Hartman <gregkh@...uxfoundation.org>,
        "containers@...ts.linux-foundation.org" 
        <containers@...ts.linux-foundation.org>,
        "stable@...r.kernel.org" <stable@...r.kernel.org>,
        "linux-kernel@...r.kernel.org" <linux-kernel@...r.kernel.org>,
        Matt Denton <mpdenton@...gle.com>, Tejun Heo <tj@...nel.org>,
        David Laight <David.Laight@...LAB.COM>,
        Al Viro <viro@...iv.linux.org.uk>,
        "linux-fsdevel@...r.kernel.org" <linux-fsdevel@...r.kernel.org>,
        "cgroups@...r.kernel.org" <cgroups@...r.kernel.org>,
        "David S . Miller" <davem@...emloft.net>
Subject: Re: [PATCH v3 1/4] fs, net: Standardize on file_receive helper to
 move fds across processes

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