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.
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.
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.
> 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@xxxxxxxxxxxxxx>
> > Cc: Christian Brauner <brauner@xxxxxxxxxx>
> > Cc: Arnd Bergmann <arnd@xxxxxxxx>
> > Signed-off-by: Günther Noack <gnoack@xxxxxxxxxx>
> > ---
> > 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.
> > +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)
> > + /*
