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

[Christian Brauner <brauner@kernel.org>]

On Wed, Mar 06, 2024 at 04:18:53PM +0100, Arnd Bergmann wrote:
> On Wed, Mar 6, 2024, at 14:47, Mickaël Salaün wrote:
> > On Tue, Mar 05, 2024 at 07:13:33PM +0100, Günther Noack wrote:
> >> On Mon, Feb 19, 2024 at 07:35:39PM +0100, Mickaël Salaün wrote:
> 
> >> > +	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);
> >> 
> >> [
> >> Technical implementation notes about this function: the list of IOCTLs here are
> >> the same ones which do_vfs_ioctl() implements directly.
> >> 
> >> There are only two cases in which do_vfs_ioctl() does more complicated handling:
> >> 
> >> (1) FIONREAD falls back to the device's ioctl implemenetation.
> >>     Therefore, we omit FIONREAD in our own list - we do not want to allow that.
> 
> >> (2) The default case falls back to the file_ioctl() function, but *only* for
> >>     S_ISREG() files, so it does not matter for the Landlock case.
> 
> How about changing do_vfs_ioctl() to return -ENOIOCTLCMD for
> FIONREAD on special files? That way, the two cases become the
> same.
> 
> >> I guess the reasons why we are not using that approach are performance, and that
> >> it might mess up the LSM hook interface with special cases that only Landlcok
> >> needs?  But it seems like it would be easier to reason about..?  Or maybe we can
> >> find a middle ground, where we have the existing hook return a special value
> >> with the meaning "permit this IOCTL, but do not invoke the f_op hook"?
> >
> > Your security_file_vfs_ioctl() approach is simpler and better, I like
> > it!  From a performance point of view it should not change much because
> > either an LSM would use the current IOCTL hook or this new one.  Using a
> > flag with the current IOCTL hook would be a missed opportunity for
> > performance improvements because this hook could be called even if it is
> > not needed.
> >
> > I don't think it would be worth it to create a new hook for compat and
> > non-compat mode because we want to control these IOCTLs the same way for
> > now, so it would not have a performance impact, but for consistency with
> > the current IOCTL hooks I guess Paul would prefer two new hooks:
> > security_file_vfs_ioctl() and security_file_vfs_ioctl_compat()?
> >
> > Another approach would be to split the IOCTL hook into two: one for the
> > VFS layer and another for the underlying implementations.  However, it
> > looks like a difficult and brittle approach according to the current
> > IOCTL implementations.
> >
> > Arnd, Christian, Paul, are you OK with this new hook proposal?
> 
> I think this sounds better. It would fit more closely into
> the overall structure of the ioctl handlers with their multiple
> levels, where below vfs_ioctl() calling into f_ops->unlocked_ioctl,
> you have the same structure for sockets and blockdev, and
> then additional levels below that and some weirdness for
> things like tty, scsi or cdrom.

So an additional security hook called from tty, scsi, or cdrom?
And the original hook is left where it is right now?