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@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.
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)
> > > + /*
>
