[PATCH v14 02/12] landlock: Add IOCTL access right for character and block devices

[Mickaël Salaün]

On Thu, Apr 18, 2024 at 11:28:00AM +0200, Günther Noack wrote:
> On Fri, Apr 12, 2024 at 05:16:59PM +0200, Mickaël Salaün wrote:
> > I like this patch very much! This patch series is in linux-next and I
> > don't expect it to change much. Just a few comments below and for test
> > patches.
> 
> Thanks!
> 
> > The only remaining question is: should we allow non-device files to
> > receive the LANDLOCK_ACCESS_FS_IOCTL_DEV right?
> 
> I think that yes, non-device files should be able to receive the
> LANDLOCK_ACCESS_FS_IOCTL_DEV right.  I played through some examples to
> ponder this:
> 
> It should be possible to grant this access right on a file hierarchy,
> for example on /dev/bus/usb to permit IOCTLs on all USB devices.
> 
> But such directories can also be empty (e.g. no USB devices plugged
> in)!  Asking user space Landlock users to traverse /dev/bus/usb to
> look for device files before using Landlock would needlessly
> complicate the API, and it would be a race condition anyway, because
> devices files can disappear at any time later as well (by unplugging
> your mouse and keyboard).
> 
> So when applies to a directory, the LANDLOCK_ACCESS_FS_IOCTL_DEV right
> already inherently needs to deal with cases where there is not a
> single device file within the directory.  (But there can technically
> be other files.)
> 
> So if the access right can be granted on any directory (with or
> without device files), it seems inconsistent that the requirements for
> using it on a file within that hierarchy should be stricter than that.

Yes, directories should be able to receive all access rights because of
file hierarchies. I was thinking about char/block devices vs.
regular/fifo/socket/symlink files.

> 
> Another data point:
> 
> This would also be consistent with the LANDLOCK_ACCESS_FS_EXECUTE
> right: This access right only has an effect on files that are marked
> executable in the first place, yet the access right can be granted on
> non-executable files as well.

I would say that LANDLOCK_ACCESS_FS_EXECUTE can be granted on fifo, but
I get your point.

> 
> To sum up:
> 
> * It seems harmless to permit, and the name of the access rights
>   already spells out that it has no effect on non-device files.
> 
> * It frees the user space libraries from doing up-front file type
>   checks.
> 
> * It would be consistent with how the access right can be granted on a
>   directory (where it really needs to be more flexible, as discussed
>   above).
> 
> * The LANDLOCK_ACCESS_FS_EXECUTE right has not been a point of
>   confusion so far, even though is has similar semantics.
> 
> So yes, I think it should be possible to use this access right on
> non-device files as well.

OK

> 
> 
> > On Fri, Apr 05, 2024 at 09:40:30PM +0000, Günther Noack wrote:
> > > Introduces the LANDLOCK_ACCESS_FS_IOCTL_DEV right
> > > and increments the Landlock ABI version to 5.
> > > 
> > > This access right applies to device-custom IOCTL commands
> > > when they are invoked on block or character device files.
> > > 
> > > Like the truncate right, this right is associated with a file
> > > descriptor at the time of open(2), and gets respected even when the
> > > file descriptor is used outside of the thread which it was originally
> > > opened in.
> > > 
> > > Therefore, a newly enabled Landlock policy does not apply to file
> > > descriptors which are already open.
> > > 
> > > If the LANDLOCK_ACCESS_FS_IOCTL_DEV right is handled, only a small
> > > number of safe IOCTL commands will be permitted on newly opened device
> > > files.  These include FIOCLEX, FIONCLEX, FIONBIO and FIOASYNC, as well
> > > as other IOCTL commands for regular files which are implemented in
> > > fs/ioctl.c.
> > > 
> > > Noteworthy scenarios which require special attention:
> > > 
> > > TTY devices are often passed into a process from the parent process,
> > > and so a newly enabled Landlock policy does not retroactively apply to
> > > them automatically.  In the past, TTY devices have often supported
> > > IOCTL commands like TIOCSTI and some TIOCLINUX subcommands, which were
> > > letting callers control the TTY input buffer (and simulate
> > > keypresses).  This should be restricted to CAP_SYS_ADMIN programs on
> > > modern kernels though.
> > > 
> > > Known limitations:
> > > 
> > > The LANDLOCK_ACCESS_FS_IOCTL_DEV access right is a coarse-grained
> > > control over IOCTL commands.
> > > 
> > > Landlock users may use path-based restrictions in combination with
> > > their knowledge about the file system layout to control what IOCTLs
> > > can be done.
> > > 
> > > Cc: Paul Moore <paul at paul-moore.com>
> > > Cc: Christian Brauner <brauner at kernel.org>
> > > Cc: Arnd Bergmann <arnd at arndb.de>
> > > Signed-off-by: Günther Noack <gnoack at google.com>
> > > ---
> > >  include/uapi/linux/landlock.h                |  38 +++-
> > >  security/landlock/fs.c                       | 221 ++++++++++++++++++-
> > 
> > You contributed a lot and you may want to add a copyright in this file.
> 
> Thanks, good point.
> 
> I'll add the Google copyright and will also retroactively add the
> copyright for the truncate contributions going back to 2022.

Good

> 
> 
> > > +static __attribute_const__ bool is_masked_device_ioctl(const unsigned int cmd)
> > > +{
> > > +   [...]
> > > +	case FICLONE:
> > > +	case FICLONERANGE:
> > > +	case FIDEDUPERANGE:
> > 
> > > +	/*
> > > +	 * FIONREAD, FS_IOC_GETFLAGS, FS_IOC_SETFLAGS, FS_IOC_FSGETXATTR and
> > > +	 * FS_IOC_FSSETXATTR are forwarded to device implementations.
> > > +	 */
> > 
> > The above comment should be better near the file_ioctl() one.
> 
> Done.
> 
> 
> > > +static __attribute_const__ bool
> > > +is_masked_device_ioctl_compat(const unsigned int cmd)
> > > +{
> > > +	switch (cmd) {
> > > +	/* FICLONE is permitted, same as in the non-compat variant. */
> > > +	case FICLONE:
> > > +		return true;
> > 
> > A new line before and after if/endif would be good.
> 
> Done.
> 
> > 
> > > +#if defined(CONFIG_X86_64)
> > > +	/*
>