[PATCH v9 1/8] landlock: Add IOCTL access right

[Günther Noack]

Thank you, Arnd and Christian, for the detailed insights!
This is very helpful!

On Mon, Feb 12, 2024 at 12:09:47PM +0100, Christian Brauner wrote:
> On Sat, Feb 10, 2024 at 12:49:23PM +0100, Arnd Bergmann wrote:
> > On Sat, Feb 10, 2024, at 12:06, Günther Noack wrote:
> > > On Fri, Feb 09, 2024 at 06:06:05PM +0100, Günther Noack wrote:
> > >
> > > The IOCTL command in question is FIONREAD: fs/ioctl.c implements
> > > FIONREAD directly for S_ISREG files, but it does call the FIONREAD
> > > implementation in the VFS layer for other file types.
> > >
> > > The question we are asking ourselves is:
> > >
> > > * Can we let processes safely use FIONREAD for all files which get
> > >   opened for the purpose of reading, or do we run the risk of
> > >   accidentally exposing surprising IOCTL implementations which have
> > >   completely different purposes?
> > >
> > >   Is it safe to assume that the VFS layer FIONREAD implementations are
> > >   actually implementing FIONREAD semantics?
> 
> Yes, otherwise this should considered a bug.

Excellent, thanks :)


> > > * I know there have been accidental collisions of IOCTL command
> > >   numbers in the past -- Hypothetically, if this were to happen in one
> > >   of the VFS implementations of FIONREAD, would that be considered a
> > >   bug that would need to get fixed in that implementation?
> > 
> > Clearly it's impossible to be sure no driver has a conflict
> > on this particular ioctl, but the risk for one intentionally
> > overriding it should be fairly low.
> > 
> > There are a couple of possible issues I can think of:
> > 
> > - the numeric value of FIONREAD is different depending
> >   on the architecture, with at least four different numbers
> >   aliasing to it. This is probably harmless but makes it
> >   harder to look for accidental conflicts.
> > 
> > - Aside from FIONREAD, it is sometimes called SIOCINQ
> >   (for sockets) or TIOCINQ (for tty). These still go
> >   through the same VFS entry point and as far as I can
> >   tell always have the same semantics (writing 4 bytes
> >   of data with the count of the remaining bytes in the
> >   fd).

Thanks, good pointer!

Grepping for these three names, I found:

* ~10 FIONREAD implementations in various drivers
* ~10 TIOCINQ implementations (all in net/, mostly in net/*/af_*.c files)
* ~20 SIOCINQ implementations (all in net/, related to network protocols?)

The implementations seem mostly related to networking, which gives me
hope that special casing and denying FIONREAD in the common case might
not make a big difference after all.

(The ioctl filtering in this patch set only applies to files opened
through open(2), but not to network sockets and other files acquired
through socket(2), pipe(2), socketpair(2), fanotiy_init(2) and the
like. -- It just gets messy if such files are opened through
/proc/*/fd/*)


> > - There are probably a couple of drivers that do something
> >   in their ioctl handler without actually looking at
> >   the command number.

Thanks you for the pointers!

You are right, it is surprisingly common that ioctl handlers do work
without first looking at the command number.  I spot checked a few
ioctl handler implementations and it is easy to dig up examples.

If this is done, the pattern is often this:

   preparation_work();
   
   switch (cmd) {
   case A:   /* impl */
   case B:   /* impl */
   default:  /* set error */
   }
   
   cleanup_work();

Common types of preparation work are the acquisition and release of
locks, allocation and release of commonly used buffers, copying memory
to and from userspace, and flushing buffers.

One of the larger examples which I found was video_ioctl2() from
drivers/media/v412-core/v412-ioctl.c, which is used from multiple
video drivers.

It also seems to me that ioctl handlers doing work independent of
command number might be a bigger problem than the hypothetical command
number collision I originally asked about.  -- If we allow FIONREAD to be called on files too liberally, we are not only exposing 


> > If you want to be really sure you get this right, you
> > could add a new callback to struct file_operations
> > that handles this for all drivers, something like
> > 
> > static int ioctl_fionread(struct file *filp, int __user *arg)
> > {
> >      int n;
> > 
> >      if (S_ISREG(inode->i_mode))
> >          return put_user(i_size_read(inode) - filp->f_pos, arg);
> > 
> >      if (!file->f_op->fionread)
> >          return -ENOIOCTLCMD;
> > 
> >      n = file->f_op->fionread(filp);
> > 
> >      if (n < 0)
> >          return n;
> > 
> >      return put_user(n, arg);
> > }
> > 
> > With this, you can go through any driver implementing
> > FIONREAD/SIOCINQ/TIOCINQ and move the code from .ioctl
> > into .fionread. This probably results in cleaner code
> > overall, especially in drivers that have no other ioctl
> > commands besides this one.
> > 
> > Since sockets and ttys tend to have both SIOCINQ/TIOCINQ
> > and SIOCOUTQ/TIOCOUTQ (unlike regular files), it's
> > probably best to do both at the same time, or maybe
> > have a single callback pointer with an in/out flag.
> 
> I'm not excited about adding a bunch of methods to struct
> file_operations for this stuff.

Agreed.  As far as I understand, if we added a special .fionread
handler to struct file_operations, the pros and cons would be:

 + For files that don't implement FIONREAD, calling ioctl(fd,
   FIONREAD, ...) could not accidentally execute ioctl handler
   preparation and cleanup work.

 - It would use a bit more space in struct file_operations.

 - It might not be obvious to driver implementers that they'd need to
   hook into .fionread.  There is a slight risk that some ioctl
   implementers would still accidentally implement FIONREAD the "old"
   way, and then it would not get called.

 - It would set a weird precedent to have a special handler for a
   single IOCTL command (why is this one command special?); this can't
   be done for all IOCTL commands like that.

If I weigh this against each other, I am not convinced that it
wouldn't cause more complications later on.  But it was a good idea
that I had not considered and it was worth looking into, I think.


In summary, I need to digest this a bit longer ;-)

I think these two were the key insights for me from this discussion:

 * There are fewer implementations of FIONREAD than I was afraid we
   would find.  This gives me some hope that we can maybe special case
   it after all in Landlock, and solve the 99% of realistic scenarios
   with it (and the remaining 1% can still ask for the "all IOCTLs"
   right, if needed).

 * Some IOCTL handlers are messier than I had expected, and it seems
   less realistic that we can convince ourselves that these are safe.
   I particularly did not realize before that ioctl handlers that
   don't even implement FIONREAD might actually still do work when
   called with FIONREAD... who would have thought! o_O

I'll try to explore the direction of special casing FIONREAD for now,
and think about it some more.  Thank you for the insightful input,
I'll loop you in when I have something more concrete.

—Günther