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?
>
> 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.)
They're not supposed to be extensible - the generic implementations are
all we need.
