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

[Casey Schaufler]

On 3/9/2024 12:14 AM, Günther Noack wrote:
> On Fri, Mar 08, 2024 at 05:25:21PM -0500, Paul Moore wrote:
>> On Fri, Mar 8, 2024 at 3:12 PM Mickaël Salaün <mic at digikod.net> wrote:
>>> On Fri, Mar 08, 2024 at 02:22:58PM -0500, Paul Moore wrote:
>>>> On Fri, Mar 8, 2024 at 4:29 AM Mickaël Salaün <mic at digikod.net> wrote:
>>>>> Let's replace "safe IOCTL" with "IOCTL always allowed in a Landlock
>>>>> sandbox".
>>>> Which is a problem from a LSM perspective as we want to avoid hooks
>>>> which are tightly bound to a single LSM or security model.  It's okay
>>>> if a new hook only has a single LSM implementation, but the hook's
>>>> definition should be such that it is reasonably generalized to support
>>>> multiple LSM/models.
>>> As any new hook, there is a first user.  Obviously this new hook would
>>> not be restricted to Landlock, it is a generic approach.  I'm pretty
>>> sure a few hooks are only used by one LSM though. ;)
>> Sure, as I said above, it's okay for there to only be a single LSM
>> implementation, but the basic idea behind the hook needs to have some
>> hope of being generic.  Your "let's redefine a safe ioctl as 'IOCTL
>> always allowed in a Landlock sandbox'" doesn't fill me with confidence
>> about the hook being generic; who knows, maybe it will be, but in the
>> absence of a patch, I'm left with descriptions like those.
> FWIW, the existing IOCTL hook is used in the following places:
>
> * TOMOYO: seemingly configurable per IOCTL command?  (I did not dig deeper)
> * SELinux: has a hardcoded switch of IOCTL commands, some with special checks.
>   These are also a subset of the do_vfs_ioctl() commands,
>   plus KDSKBENT, KDSKBSENT (from ioctl_console(2)).
> * Smack: Decomposes the IOCTL command number to look at the _IOC_WRITE and
>   _IOC_READ bits. (This is a known problematic approach, because (1) these bits
>   describe whether the argument is getting read or written, not whether the
>   operation is a mutating one, and (2) some IOCTL commands do not adhere to the
>   convention and don't use these macros)

These shortcomings are well understood. It's a whole lot better than what was
done originally, but definitely not up to formal scrutiny. Back in the bad old
days of UNIX security evaluations we spent as much time on ioctl() as we did
on the rest of the system. Or so it seemed.

>
> AppArmor does not use the LSM IOCTL hook.
>
>
>>>> I understand that this makes things a bit more
>>>> complicated for Landlock's initial ioctl implementation, but
>>>> considering my thoughts above and the fact that Landlock's ioctl
>>>> protections are still evolving I'd rather not add a lot of extra hooks
>>>> right now.
>>> Without this hook, we'll need to rely on a list of allowed IOCTLs, which
>>> will be out-of-sync eventually.  It would be a maintenance burden and an
>>> hacky approach.
>> Welcome to the painful world of a LSM developer, ioctls are not the
>> only place where this is a problem, and it should be easy enough to
>> watch for changes in the ioctl list and update your favorite LSM
>> accordingly.  Honestly, I think that is kinda the right thing anyway,
>> I'm skeptical that one could have a generic solution that would
>> automatically allow or disallow a new ioctl without potentially
>> breaking your favorite LSM's security model.  If a new ioctl is
>> introduced it seems like having someone manually review it's impact on
>> your LSM would be a good idea.
> We are concerned that we will miss a change in do_vfs_ioctl(), which we would
> like to reflect in the matching Landlock code.  Do other LSMs have any
> approaches for that which go beyond just watching the do_vfs_ioctl()
> implementation for changes?
>
>
>>> We're definitely open to new proposals, but until now this is the best
>>> approach we found from a maintenance, performance, and security point of
>>> view.
>> At this point it's probably a good idea to post another RFC patch with
>> your revised idea, if nothing else it will help rule out any
>> confusion.  While I remain skeptical, perhaps I am misunderstanding
>> the design and you'll get my apology and an ACK, but be warned that as
>> of right now I'm not convinced.
> Thanks you for your feedback!
>
> Here is V10 with the approach where we use a new LSM hook:
> https://lore.kernel.org/all/20240309075320.160128-1-gnoack@google.com/
>
> I hope this helps to clarify the approach a bit.  I'm explaining it in more
> detail again in the commit which adds the LSM hook, including a call graph, and
> avoiding the word "safe" this time ;-)
>
> Let me know what you think!
>
> Thanks!
> —Günther
>