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

On Thu, Jun 11, 2020 at 11:19:42AM +0200, Christian Brauner wrote:

On Wed, Jun 10, 2020 at 07:59:55PM -0700, Kees Cook wrote:

quoted

On Wed, Jun 10, 2020 at 08:12:38AM +0000, Sargun Dhillon wrote:

quoted

As an aside, all of this junk should be dropped:
+	ret = get_user(size, &uaddfd->size);
+	if (ret)
+		return ret;
+
+	ret = copy_struct_from_user(&addfd, sizeof(addfd), uaddfd, size);
+	if (ret)
+		return ret;

and the size member of the seccomp_notif_addfd struct. I brought this up 
off-list with Tycho that ioctls have the size of the struct embedded in them. We 
should just use that. The ioctl definition is based on this[2]:
#define _IOC(dir,type,nr,size) \
	(((dir)  << _IOC_DIRSHIFT) | \
	 ((type) << _IOC_TYPESHIFT) | \
	 ((nr)   << _IOC_NRSHIFT) | \
	 ((size) << _IOC_SIZESHIFT))


We should just use copy_from_user for now. In the future, we can either 
introduce new ioctl names for new structs, or extract the size dynamically from 
the ioctl (and mask it out on the switch statement in seccomp_notify_ioctl.

Yeah, that seems reasonable. Here's the diff for that part:

diff --git a/include/uapi/linux/seccomp.h b/include/uapi/linux/seccomp.h
index 7b6028b399d8..98bf19b4e086 100644
--- a/include/uapi/linux/seccomp.h
+++ b/include/uapi/linux/seccomp.h

@@ -118,7 +118,6 @@ struct seccomp_notif_resp {
 
 /**
  * struct seccomp_notif_addfd
- * @size: The size of the seccomp_notif_addfd datastructure
  * @id: The ID of the seccomp notification
  * @flags: SECCOMP_ADDFD_FLAG_*
  * @srcfd: The local fd number

@@ -126,7 +125,6 @@ struct seccomp_notif_resp {
  * @newfd_flags: The O_* flags the remote FD should have applied
  */
 struct seccomp_notif_addfd {
-	__u64 size;
 	__u64 id;
 	__u32 flags;
 	__u32 srcfd;

diff --git a/kernel/seccomp.c b/kernel/seccomp.c
index 3c913f3b8451..00cbdad6c480 100644
--- a/kernel/seccomp.c
+++ b/kernel/seccomp.c

@@ -1297,14 +1297,9 @@ static long seccomp_notify_addfd(struct seccomp_filter *filter,
 	struct seccomp_notif_addfd addfd;
 	struct seccomp_knotif *knotif;
 	struct seccomp_kaddfd kaddfd;
-	u64 size;
 	int ret;
 
-	ret = get_user(size, &uaddfd->size);
-	if (ret)
-		return ret;
-
-	ret = copy_struct_from_user(&addfd, sizeof(addfd), uaddfd, size);
+	ret = copy_from_user(&addfd, uaddfd, sizeof(addfd));
 	if (ret)
 		return ret;
 

quoted

----
+#define SECCOMP_IOCTL_NOTIF_ADDFD	SECCOMP_IOR(3,	\
+						struct seccomp_notif_addfd)

Lastly, what I believe to be a small mistake, it should be SECCOMP_IOW, based on 
the documentation in ioctl.h -- "_IOW means userland is writing and kernel is 
reading."

Oooooh. Yeah; good catch. Uhm, that means SECCOMP_IOCTL_NOTIF_ID_VALID
is wrong too, yes? Tycho, Christian, how disruptive would this be to
fix? (Perhaps support both and deprecate the IOR version at some point
in the future?)

We have custom defines in our source code, i.e.
#define SECCOMP_IOCTL_NOTIF_ID_VALID  SECCOMP_IOR(2, __u64)
so ideally we'd have a SECCOMP_IOCTL_NOTIF_ID_VALID_V2

Does that sound ok?

Christian

Why not change the public API in seccomp.h to:
#define SECCOMP_IOCTL_NOTIF_ID_VALID	SECCOMP_IOW(2, __u64)

And then in seccomp.c:
#define SECCOMP_IOCTL_NOTIF_ID_VALID_OLD	SECCOMP_IOR(2, __u64)
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;

	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_OLD:
		pr_warn_once("Detected usage of legacy (incorrect) version of seccomp notifier notif_id_valid ioctl\n");
	case SECCOMP_IOCTL_NOTIF_ID_VALID:
		return seccomp_notify_id_valid(filter, buf);
	default:
		return -EINVAL;
	}
}
---- 

So, both will work fine, and whenevery anyone recompiles, or picks up new 
headers, they will start calling the "right" one without a code change, and
we wont break any userspace.