Re: [RFC PATCH] fs: Add vfs_masks_device_ioctl*() helpers

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

On Mon, Mar 11, 2024 at 12:03:13PM +1100, Dave Chinner wrote:
> On Fri, Mar 08, 2024 at 12:03:01PM +0100, Günther Noack wrote:
> > On Fri, Mar 08, 2024 at 08:02:13AM +0100, Arnd Bergmann wrote:
> > > On Fri, Mar 8, 2024, at 00:09, Dave Chinner wrote:
> > > > I have no idea what a "safe" ioctl means here. Subsystems already
> > > > restrict ioctls that can do damage if misused to CAP_SYS_ADMIN, so
> > > > "safe" clearly means something different here.
> > > 
> > > That was my problem with the first version as well, but I think
> > > drawing the line between "implemented in fs/ioctl.c" and
> > > "implemented in a random device driver fops->unlock_ioctl()"
> > > seems like a more helpful definition.
> > 
> > Yes, sorry for the confusion - that is exactly what I meant to say with "safe".:
> > 
> > Those are the IOCTL commands implemented in fs/ioctl.c which do not go through
> > f_ops->unlocked_ioctl (or the compat equivalent).
> 
> Which means all the ioctls we wrequire for to manage filesystems are
> going to be considered "unsafe" and barred, yes?
> 
> That means you'll break basic commands like 'xfs_info' that tell you
> the configuration of the filesystem. It will prevent things like
> online growing and shrinking, online defrag, fstrim, online
> scrubbing and repair, etc will not worki anymore. It will break
> backup utilities like xfsdump, and break -all- the device management
> of btrfs and bcachefs filesystems.
> 
> Further, all the setup and management of -VFS functionality- like
> fsverity and fscrypt is actually done at the filesystem level (i.e
> through ->unlocked_ioctl, no do_vfs_ioctl()) so those are all going
> to get broken as well despite them being "vfs features".
> 
> Hence from a filesystem perspective, this is a fundamentally
> unworkable definition of "safe".

As discussed further up in this thread[1], we want to only apply the IOCTL
command filtering to block and character devices.  I think this should resolve
your concerns about file system specific IOCTLs?  This is implemented in patch
V10 going forward[2].

[1] https://lore.kernel.org/all/20240219.chu4Yeegh3oo@digikod.net/
[2] https://lore.kernel.org/all/20240309075320.160128-1-gnoack@google.com/


> > We want to give people a way with Landlock so that they can restrict the use of
> > device-driver implemented IOCTLs, but where they can keep using the bulk of
> > more harmless IOCTLs in fs/ioctl.c.
> 
> Hah! There's plenty of "harm" that can be done through those ioctls.
> It's the entry point for things like filesystem freeze/thaw, FIEMAP
> (returns physical data location information), file cloning,
> deduplication and per-inode feature manipulation. Lots of this stuff
> is under CAP_SYS_ADMIN because they aren't safe for to be exposed to
> general users...

The operations themselves are not all harmless, but they are harmless to permit
from the Landlock perspective, because (as you point out as well) their use is
already adequately controlled in their existing implementations.

The proposed patch v10 only influences IOCTL operations on device files,
so the "reflink" deduplication IOCTLs, FIEMAP, etc. should not matter.

—Günther