[PATCH v10 1/9] security: Create security_file_vfs_ioctl hook

[Paul Moore]

On Sat, Mar 9, 2024 at 2:53 AM Günther Noack <gnoack at google.com> wrote:
>
> This LSM hook gets called just before the fs/ioctl.c logic delegates
> the requested IOCTL command to the file-specific implementation as
> implemented by f_op->unlocked_ioctl (or f_op->ioctl_compat).
>
> It is impractical for LSMs to make security guarantees about these
> f_op operations without having intimate knowledge of how they are
> implemented.
>
> Therefore, depending on the enabled Landlock policy, Landlock aims to
> block the calls to filp->f_op->unlocked_ioctl(), but permit the calls
> to the IOCTL commands which are already implemented in fs/ioctl.c.
>
> The current call graph is:
>
>   * ioctl syscall
>     * security_file_ioctl() LSM hook
>     * do_vfs_ioctl() - standard operations
>       * file_ioctl() - standard file operations
>     * vfs_ioctl() - delegate to file (if do_vfs_ioctl() is a no-op)
>       * filp->f_op->unlocked_ioctl()
>
> Why not use the existing security_file_ioctl() hook?
>
> With the existing security_file_ioctl() hook, it is technically
> feasible to prevent the call to filp->f_op->unlocked_ioctl(), but it
> would be difficult to maintain: security_file_ioctl() gets called
> further up the call stack, so an implementation of it would need to
> predict whether the logic further below will decide to call
> f_op->unlocked_ioctl().  That can only be done by mirroring the logic
> in do_vfs_ioctl() to some extent, and keeping this in sync.

Once again, I don't see this as an impossible task, and I would think
that you would want to inspect each new ioctl command/op added in
do_vfs_ioctl() anyway to ensure it doesn't introduce an unwanted
behavior from a Landlock sandbox perspective.  Looking at the git
log/blame, it also doesn't appear that new do_vfs_ioctl() ioctls are
added very frequently, meaning that keeping Landlock sync'd with
fs/ioctl.c shouldn't be a terrible task.

I'm also not excited about the overlap between the existing
security_file_ioctl() hook and the proposed security_file_vfs_ioctl()
hook.  There are some cases where we have no choice and we have to
tolerate the overlap, but this doesn't look like one of those cases to
me.

I'm sorry, but I don't agree with this new hook.

-- 
paul-moore.com