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

["Arnd Bergmann" <arnd@arndb.de>]

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.

>> And there is a scenario where this could potentially happen:
>> 
>> do_vfs_ioctl() implements most things like this:
>> 
>> static int do_vfs_ioctl(...) {
>> 	switch (cmd) {
>> 	/* many cases like the following: */
>> 	case FITHAW:
>> 		return ioctl_fsthaw(filp);
>> 	/* ... */
>> 	}
>> 	return -ENOIOCTLCMD;
>> }
>> 
>> So I believe the scenario you want to avoid is the one where ioctl_fsthaw() or
>> one of the other functions return -ENOIOCTLCMD by accident, and where that will
>> then make the surrounding syscall implementation fall back to vfs_ioctl()
>> despite the cmd being listed as safe for Landlock?  Is that right?
>
> Yes

This does go against the normal structure a bit then, where
any of the commands is allowed to return -ENOIOCTLCMD specifically
for the purpose of passing control to the next level of
callbacks. Having the landlock hook explicitly at the
place where the callback is entered, as Günther suggested makes
much more sense to me then.

      Arnd