On Fri, Mar 01, 2024 at 05:24:52PM +0100, Arnd Bergmann wrote:
> On Mon, Feb 19, 2024, at 19:35, Mickaël Salaün wrote:
> > vfs_masks_device_ioctl() and vfs_masks_device_ioctl_compat() are useful
> > to differenciate between device driver IOCTL implementations and
> > filesystem ones. The goal is to be able to filter well-defined IOCTLs
> > from per-device (i.e. namespaced) IOCTLs and control such access.
> >
> > Add a new ioctl_compat() helper, similar to vfs_ioctl(), to wrap
> > compat_ioctl() calls and handle error conversions.
>
> I'm still a bit confused by what your goal is here. I see
> the code getting more complex but don't see the payoff in this
> patch.
The main idea is to be able to identify if an IOCTL is handled by a
device driver (i.e. block or character devices) or not. This would be
used at least by Landlock to control such IOCTL (according to the
char/block device file, which can already be identified) while allowing
other VFS/FS IOCTLs.
>
> > Cc: Arnd Bergmann <arnd@xxxxxxxx>
> > Cc: Christian Brauner <brauner@xxxxxxxxxx>
> > Cc: Günther Noack <gnoack@xxxxxxxxxx>
>
> I assume the missing Signed-off-by is intentional while you are
> still gathering feedback?
No, I sent it too quickly and forgot to add it.
Signed-off-by: Mickaël Salaün <mic@xxxxxxxxxxx>
>
> > +/*
> > + * Safeguard to maintain a list of valid IOCTLs handled by
> > do_vfs_ioctl()
> > + * instead of def_blk_fops or def_chr_fops (see init_special_inode).
> > + */
> > +__attribute_const__ bool vfs_masked_device_ioctl(const unsigned int
> > cmd)
> > +{
> > + switch (cmd) {
> > + case FIOCLEX:
> > + case FIONCLEX:
> > + case FIONBIO:
> > + case FIOASYNC:
> > + case FIOQSIZE:
> > + case FIFREEZE:
> > + case FITHAW:
> > + case FS_IOC_FIEMAP:
> > + case FIGETBSZ:
> > + case FICLONE:
> > + case FICLONERANGE:
> > + case FIDEDUPERANGE:
> > + /* FIONREAD is forwarded to device implementations. */
> > + case FS_IOC_GETFLAGS:
> > + case FS_IOC_SETFLAGS:
> > + case FS_IOC_FSGETXATTR:
> > + case FS_IOC_FSSETXATTR:
> > + /* file_ioctl()'s IOCTLs are forwarded to device implementations. */
> > + return true;
> > + default:
> > + return false;
> > + }
> > +}
> > +EXPORT_SYMBOL(vfs_masked_device_ioctl);
>
> It looks like this gets added into the hot path of every
> ioctl command, which is not ideal, especially when this
> no longer gets inlined into the caller.
I'm looking for a way to guarantee that the list of IOCTLs in this
helper will be kept up-to-date. This kind of run time check might be
too much though. Do you have other suggestions? Do you think a simple
comment to remind contributors to update this helper would be enough?
I guess VFS IOCTLs should not be added often, but I'm worried that this
list could get out of sync...
>
> > + inode = file_inode(f.file);
> > + is_device = S_ISBLK(inode->i_mode) || S_ISCHR(inode->i_mode);
> > + if (is_device && !vfs_masked_device_ioctl(cmd)) {
> > + error = vfs_ioctl(f.file, cmd, arg);
> > + goto out;
> > + }
>
> The S_ISBLK() || S_ISCHR() check here looks like it changes
> behavior, at least for sockets. If that is intentional,
> it should probably be a separate patch with a detailed
> explanation.
I don't think this changes the behavior for sockets, and at least that's
not intentionnal. This patch should not change the current behavior at
all.
The path to reach socket IOCTLs goes through a vfs_ioctl() call, which
is...
>
> > error = do_vfs_ioctl(f.file, fd, cmd, arg);
> > - if (error == -ENOIOCTLCMD)
> > + if (error == -ENOIOCTLCMD) {
> > + WARN_ON_ONCE(is_device);
> > error = vfs_ioctl(f.file, cmd, arg);
...here!
> > + }
>
> The WARN_ON_ONCE() looks like it can be triggered from
> userspace, which is generally a bad idea.
This WARN_ON_ONCE() should never be triggered because if the it is a
device IOCTL it goes through the previous vfs_ioctl() (for
device-specific command) call or through this do_vfs_ioctl() call (for
VFS-specific command).
>
> > +extern __attribute_const__ bool vfs_masked_device_ioctl(const unsigned
> > int cmd);
> > +#ifdef CONFIG_COMPAT
> > +extern __attribute_const__ bool
> > +vfs_masked_device_ioctl_compat(const unsigned int cmd);
> > +#else /* CONFIG_COMPAT */
> > +static inline __attribute_const__ bool
> > +vfs_masked_device_ioctl_compat(const unsigned int cmd)
> > +{
> > + return vfs_masked_device_ioctl(cmd);
> > +}
> > +#endif /* CONFIG_COMPAT */
>
> I don't understand the purpose of the #else path here,
> this should not be needed.
Correct, this else branch is useless.
>
> Arnd
>
