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

[David Laight <David.Laight@ACULAB.COM>]

From: Kees Cook
> Sent: 12 June 2020 00:50
> > From: Sargun Dhillon
> > > Sent: 11 June 2020 12:07
> > > Subject: Re: [PATCH v3 1/4] fs, net: Standardize on file_receive helper to move fds across
> processes
> > >
> > > On Thu, Jun 11, 2020 at 12:01:14PM +0200, Christian Brauner wrote:
> > > > On Wed, Jun 10, 2020 at 07:59:55PM -0700, Kees Cook wrote:
> > > > > On Wed, Jun 10, 2020 at 08:12:38AM +0000, Sargun Dhillon wrote:
> > > > > > 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)