From: Kees Cook

Sent: 12 June 2020 00:50

quoted

From: Sargun Dhillon

quoted

Sent: 11 June 2020 12:07
Subject: Re: [PATCH v3 1/4] fs, net: Standardize on file_receive helper to move fds across

processes

quoted

quoted

On Thu, Jun 11, 2020 at 12:01:14PM +0200, Christian Brauner wrote:

quoted

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:

Why does it matter that the ioctl() has the size of the struct embedded
within? Afaik, the kernel itself doesn't do anything with that size. It
merely checks that the size is not pathological and it does so at
compile time.

#ifdef __CHECKER__
#define _IOC_TYPECHECK(t) (sizeof(t))
#else
/* provoke compile error for invalid uses of size argument */
extern unsigned int __invalid_size_argument_for_IOC;
#define _IOC_TYPECHECK(t) \
	((sizeof(t) == sizeof(t[1]) && \
	  sizeof(t) < (1 << _IOC_SIZEBITS)) ? \
	  sizeof(t) : __invalid_size_argument_for_IOC)
#endif

The size itself is not verified at runtime. copy_struct_from_user()
still makes sense at least if we're going to allow expanding the struct
in the future.

Right, but if we simply change our headers and extend the struct, it will break
all existing programs compiled against those headers. In order to avoid that, if
we intend on extending this struct by appending to it, we need to have a
backwards compatibility mechanism. Just having copy_struct_from_user isn't
enough. The data structure either must be fixed size, or we need a way to handle
multiple ioctl numbers derived from headers with different sized struct arguments

The two approaches I see are:
1. use more indirection. This has previous art in drm[1]. That's look
something like this:

struct seccomp_notif_addfd_ptr {
	__u64 size;
	__u64 addr;
}

... And then it'd be up to us to dereference the addr and copy struct from user.

Do not go down that route. It isn't worth the pain.

You should also assume that userspace might have a compile-time check
on the buffer length (I've written one - not hard) and that the kernel
might (in the future - or on a BSD kernel) be doing the user copies
for you.

Also, if you change the structure you almost certainly need to
change the name of the ioctl cmd as well as its value.
Otherwise a recompiled program will pass the new cmd value (and
hopefully the right sized buffer) but it won't have initialised
the buffer properly.
This is likely to lead to unexpected behaviour.

Hmmm.

So, while initially I thought Sargun's observation about ioctl's fixed
struct size was right, I think I've been swayed to Christian's view
(which is supported by the long tail of struct size pain we've seen in
other APIs).

Doing a separate ioctl for each structure version seems like the "old
solution" now that we've got EA syscalls. So, I'd like to keep the size
and copy_struct_from_user().

If the size is variable then why not get the application to fill
in the size of the structure it is sending at the time of the ioctl.

So you'd have:
#define xxx_IOCTL_17(param) _IOCW('X', 17, sizeof *(param))

The application code would then do:
	ioctl(fd, xxx_IOCTL_17(arg), arg);

The kernel code can either choose to have specific 'case'
for each size, or mask off the length bits and do the
length check later.

	David

-
Registered Address Lakeside, Bramley Road, Mount Farm, Milton Keynes, MK1 1PT, UK
Registration No: 1397386 (Wales)