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?
https://lore.kernel.org/lkml/CAOQ4uxjvEL4P4vV5SKpHVS5DtOwKpxAn4n4+Kfqawcu+H-MC5g@xxxxxxxxxxxxxx/
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
