Re: [PATCH v13 01/10] landlock: Add IOCTL access right for character and block devices

["Mickaël Salaün" <mic@digikod.net>]

On Fri, Apr 05, 2024 at 06:22:52PM +0200, Günther Noack wrote:
> On Fri, Apr 05, 2024 at 06:17:17PM +0200, Günther Noack wrote:
> > On Wed, Apr 03, 2024 at 01:15:45PM +0200, Mickaël Salaün wrote:
> > > On Tue, Apr 02, 2024 at 08:28:49PM +0200, Günther Noack wrote:
> > > > Can you please clarify how you make up your mind about what should be permitted
> > > > and what should not?  I have trouble understanding the rationale for the changes
> > > > that you asked for below, apart from the points that they are harmless and that
> > > > the return codes should be consistent.
> > > 
> > > The rationale is the same: all IOCTL commands that are not
> > > passed/specific to character or block devices (i.e. IOCTLs defined in
> > > fs/ioctl.c) are allowed.  vfs_masked_device_ioctl() returns true if the
> > > IOCTL command is not passed to the related device driver but handled by
> > > fs/ioctl.c instead (i.e. handled by the VFS layer).
> > 
> > Thanks for clarifying -- this makes more sense now.  I traced the cases with
> > -ENOIOCTLCMD through the code more thoroughly and it is more aligned now with
> > what you implemented before.  The places where I ended up implementing it
> > differently to your vfs_masked_device_ioctl() patch are:
> > 
> >  * Do not blanket-permit FS_IOC_{GET,SET}{FLAGS,XATTR}.
> >    They fall back to the device implementation.
> > 
> >  * FS_IOC_GETUUID and FS_IOC_GETFSSYSFSPATH are now handled.
> >    These return -ENOIOCTLCMD from do_vfs_ioctl(), so they do fall back to the
> >    handlers in struct file_operations, so we can not permit these either.
> 
> Kent, Amir:
> 
> Is it intentional that the new FS_IOC_GETUUID and FS_IOC_GETFSSYSFSPATH IOCTLs
> can fall back to a IOCTL implementation in struct file_operations?  I found this
> remark by Amir which sounded vaguely like it might have been on purpose?  Did I
> understand that correctly?

I think the rationale is that all new VFS IOCTLs should have this fall
back because device drivers might already implement them.

> 
> https://lore.kernel.org/lkml/CAOQ4uxjvEL4P4vV5SKpHVS5DtOwKpxAn4n4+Kfqawcu+H-MC5g@mail.gmail.com/
> 
> Otherwise, I am happy to send a patch to make it non-extensible (the impls in
> fs/ioctl.c would need to return -ENOTTY).  This would let us reason better about
> the safety of these IOCTLs for IOCTL security policies enforced by the Landlock
> LSM. (Some of these file_operations IOCTL implementations do stuff before
> looking at the cmd number.)
> 
> Thanks,
> —Günther
>