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

From: Kees Cook <keescook@chromium.org>
To: Sargun Dhillon <sargun@sargun.me>
Cc: David Laight <David.Laight@ACULAB.COM>,
	Christian Brauner <christian.brauner@ubuntu.com>,
	"containers@lists.linux-foundation.org" 
	<containers@lists.linux-foundation.org>,
	Giuseppe Scrivano <gscrivan@redhat.com>,
	Robert Sesek <rsesek@google.com>,
	Chris Palmer <palmer@google.com>, Jann Horn <jannh@google.com>,
	Greg Kroah-Hartman <gregkh@linuxfoundation.org>,
	"linux-kernel@vger.kernel.org" <linux-kernel@vger.kernel.org>,
	Matt Denton <mpdenton@google.com>,
	"linux-fsdevel@vger.kernel.org" <linux-fsdevel@vger.kernel.org>,
	Tejun Heo <tj@kernel.org>, Al Viro <viro@zeniv.linux.org.uk>,
	"cgroups@vger.kernel.org" <cgroups@vger.kernel.org>,
	"stable@vger.kernel.org" <stable@vger.kernel.org>,
	"David S . Miller" <davem@davemloft.net>
Subject: Re: [PATCH v3 1/4] fs, net: Standardize on file_receive helper to move fds across processes
Date: Fri, 12 Jun 2020 08:13:25 -0700	[thread overview]
Message-ID: <202006120806.E770867EF@keescook> (raw)
In-Reply-To: <20200612104629.GA15814@ircssh-2.c.rugged-nimbus-611.internal>

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