Re: [PATCH v3 0/5] Landlock: IOCTL support

["Günther Noack" <gnoack@google.com>]

Hi!

On Fri, Aug 18, 2023 at 03:39:19PM +0200, Mickaël Salaün wrote:
> On Mon, Aug 14, 2023 at 07:28:11PM +0200, Günther Noack wrote:
> > These patches add simple ioctl(2) support to Landlock.
> 
> [...]
> 
> > How we arrived at the list of always-permitted IOCTL commands
> > ~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~
> > 
> > To decide which IOCTL commands should be blanket-permitted I went through the
> > list of IOCTL commands mentioned in fs/ioctl.c and looked at them individually
> > to understand what they are about.  The following list is my conclusion from
> > that.
> > 
> > We should always allow the following IOCTL commands:
> > 
> >  * FIOCLEX, FIONCLEX - these work on the file descriptor and manipulate the
> >    close-on-exec flag
> >  * FIONBIO, FIOASYNC - these work on the struct file and enable nonblocking-IO
> >    and async flags
> >  * FIONREAD - get the number of bytes available for reading (the implementation
> >    is defined per file type)
> 
> I think we should treat FIOQSIZE like FIONREAD, i.e. check for
> LANDLOCK_ACCESS_FS_READ_FILE as explain in my previous message.
> Tests should then rely on something else.

OK, I rewrote the tests to use FS_IOC_GETFLAGS.

Some thoughts on these two IOCTLs:

FIONREAD gives the number of bytes that are ready to read.  This IOCTL seems
only useful when the file is open for reading.  However, do you think that we
should correlate this with (a) LANDLOCK_ACCESS_FS_READ_FILE, or with (b)
f->f_mode & FMODE_READ?  (The difference is that in case (a), FIONREAD will work
if you open a file O_WRONLY and you also have the LANDLOCK_ACCESS_FS_READ_FILE
right for that file.  In case (b), it would only work if you also opened the
file for reading.)

FIOQSIZE seems like it would be useful for both reading *and* writing? -- The
reading case is obvious, but for writers it's also useful if you want to seek
around in the file, and make sure that the position that you seek to already
exists.  (I'm not sure whether that use case is relevant in practical
applications though.) -- Why would FIOQSIZE only be useful for readers?

(In fact, it seems to me almost like FIOQSIZE might rather be missing a security
hook check for one of the "getting file attribute" hooks?)

So basically, the implementation that I currently ended up with is:

switch (cmd) {
  case FIOCLEX:
  case FIONCLEX:
  case FIONBIO:
  case FIOASYNC:
  case FIOQSIZE:
    return 0;
  case FIONREAD:
    if (file->f_mode & FMODE_READ)
      return 0;
}

(with some comments in the source code, of course...)

Does that look reasonable to you?

—Günther

-- 
Sent using Mutt 🐕 Woof Woof