Re: [RFC PATCH 6/9] Creating supervisor events for filesystem operations

["Mickaël Salaün" <mic@digikod.net>]

On Tue, Mar 04, 2025 at 01:13:02AM +0000, Tingmao Wang wrote:
> NOTE from future me: This implementation which waits for user response
> while blocking inside the current security_path_* hooks is problematic due
> to taking exclusive inode lock on the parent directory, and while I have a
> proposal for a solution, outlined below, I haven't managed to include the
> code for that in this version of the patch. Thus for this commit in
> particular I'm probably more looking for suggestions on the approach
> rather than code review.  Please see the TODO section at the end of this
> message before reviewing this patch.

This is good for an RFC.

> 
> ----
> 
> This patch implements a proof-of-concept for modifying the current
> landlock LSM hooks to send supervisor events and wait for responses, when
> a supervised layer is involved.
> 
> In this design, access requests which would end up being denied by other
> non-supervised landlock layers (or which would fail the normal inode
> permission check anyways - but this is currently TODO, I only thought of
> this afterwards) are denied straight away to avoid pointless supervisor
> notifications.

Yes, only denied access should be forwarded to the supervisor.  In
another patch series we could enable the supervisor to update its layer
with new rules as well.

The audit patch series should help to properly identify which layer
denied a request, and to only use the related supervisor.

> 
> Currently current_check_access_path only gets the path of the parent
> directory for create/remove operations, which is not enough for what we
> want to pass to the supervisor.  Therefore we extend it by passing in any
> relevant child dentry (but see TODO below - this may not be possible with
> the proper implementation).

Hmm, I'm not sure this kind of information is required (this is not
implemented for the audit support).  The supervisor should be fine
getting only which access is missing, right?

> 
> This initial implementation doesn't handle links and renames, and for now
> these operations behave as if no supervisor is present (and thus will be
> denied, unless it is allowed by the layer rules).  Also note that we can
> get spurious create requests if the program tries to O_CREAT open an
> existing file that exists but not in the dcache (from my understanding).
> 
> Event IDs (referred to as an opaque cookie in the uapi) are currently
> generated with a simple `next_event_id++`.  I considered using e.g. xarray
> but decided to not for this PoC. Suggestions welcome. (Note that we have
> to design our own event id even if we use an extension of fanotify, as
> fanotify uses a file descriptor to identify events, which is not generic
> enough for us)

That's another noticable difference with fanotify.  You can add it to
the next cover letter.

> 
> ----
> 
> TODO:
> 
> When testing this I realized that doing it this way means that for the
> create/delete case, we end up holding an exclusive inode lock on the
> parent directory while waiting for supervisor to respond (see namei.c -
> security_path_mknod is called in may_o_create <- lookup_open which has an
> exclusive lock if O_CREAT is passed), which will prevent all other tasks
> from accessing that directory (regardless of whether or not they are under
> landlock).

Could we use a landlock_object to identify this inode instead?

> 
> This is clearly unacceptable, but since landlock (and also this extension)
> doesn't actually need a dentry for the child (which is allocated after the
> inode lock), I think this is not unsolvable.  I'm experimenting with
> creating a new LSM hook, something like security_pathname_mknod
> (suggestions welcome), which will be called after we looked up the dentry
> for the parent (to prevent racing symlinks TOCTOU), but before we take the
> lock for it.  Such a hook can still take as argument the parent dentry,
> plus name of the child (instead of a struct path for it).
> 
> Suggestions for alternative approaches are definitely welcome!
> 
> Signed-off-by: Tingmao Wang <m@maowtm.org>
> ---
>  security/landlock/fs.c        | 134 ++++++++++++++++++++++++++++++++--
>  security/landlock/supervise.c | 122 +++++++++++++++++++++++++++++++
>  security/landlock/supervise.h | 106 ++++++++++++++++++++++++++-
>  3 files changed, 354 insertions(+), 8 deletions(-)
> 

[...]