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

[Christian Brauner <brauner@kernel.org>]

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.

> >
> > * 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).
> 
> - There are probably a couple of drivers that do something
>   in their ioctl handler without actually looking at
>   the command number.
> 
> 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

I'm not excited about adding a bunch of methods to struct
file_operations for this stuff.