Re: [PATCH v4 0/7] Landlock: IOCTL support

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

On Thu, Nov 30, 2023 at 10:26:47AM +0100, Mickaël Salaün wrote:
> On Fri, Nov 24, 2023 at 02:02:12PM +0100, Günther Noack wrote:
> > On Fri, Nov 17, 2023 at 09:44:31PM +0100, Mickaël Salaün wrote:
> > I don't think it is at odds with the backwards-compatibility concerns which we
> > previously discussed.  The synthetic groups still exist, it's just the
> > "permitting LANDLOCK_ACCESS_FS_IOCTL on a file or directory" which affects a
> > different set of IOCTL commands.
> 
> It would not be a backward-compatibility issue, but it would make
> LANDLOCK_ACCESS_FS_IOCTL too powerful even if we get safer/better access
> rights. Furthermore, reducing the scope of LANDLOCK_ACCESS_FS_IOCTL with
> new handled access rights should better inform end encourage developers
> to drop LANDLOCK_ACCESS_FS_IOCTL when it is not strictly needed.
> It would be useful to explain this rationale in the commit message.
> See https://lore.kernel.org/all/20231020.moefooYeV9ei@digikod.net/

Done; I added a section to the commit message.


> > > > > If the scope of LANDLOCK_ACCESS_FS_IOCTL is well documented, that should
> > > > > be OK. But maybe we should rename this right to something like
> > > > > LANDLOCK_ACCESS_FS_IOCTL_DEFAULT to make it more obvious that it handles
> > > > > IOCTLs that are not handled by other access rights?
> > > > 
> > > > Hmm, I'm not convinced this is a good name.  It makes sense in the context of
> > > > allowing "all the other ioctls" for a file or file hierarchy, but when setting
> > > > LANDLOCK_ACCESS_FS_IOCTL in handled_access_fs, that flag turns off *all* ioctls,
> > > > so "default" doesn't seem appropriate to me.
> > > 
> > > It should turn off all IOCTLs that are not handled by another access
> > > right.  The handled access rights should be expanded the same way as the
> > > allowed access rights.
> > 
> > If you handle LANDLOCK_ACCESS_FS_IOCTL, and you don't permit anything on files
> > or directories, all IOCTL commands will be forbidden, independent of what else
> > is handled.
> > 
> > The opposite is not true:
> > 
> > If you handle LANDLOCK_ACCESS_FS_READ_FILE, and you don't handle
> > LANDLOCK_ACCESS_FS_IOCTL, all IOCTL commands will happily work.
> > 
> > So if you see it through that lens, you could say that it is only the
> > LANDLOCK_ACCESS_FS_IOCTL bit in the "handled" mask which forbids any IOCTL
> > commands at all.
> 
> Handling LANDLOCK_ACCESS_FS_IOCTL does make all IOCTLs denied by
> default. However, to allow IOCTLs, developers may need to allow
> LANDLOCK_ACCESS_FS_IOCTL or another access right according to the
> underlying semantic.
> 
> It would be useful to add an example with your table in the
> documentation, for instance with LANDLOCK_ACCESS_FS_READ_FILE (handled
> or not handled, with LANDLOCK_ACCESS_FS_IOCTL or not, allowed on a file
> or not...).

Added an example to the documentation.

—Günther