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:

quoted

On Sat, Feb 10, 2024, at 12:06, Günther Noack wrote:

quoted

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 :)

quoted

quoted

* 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/*)

quoted

- 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 

quoted

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