On Tue, Mar 11, 2025 at 12:42:05AM +0000, Tingmao Wang wrote:

On 3/6/25 17:07, Amir Goldstein wrote:
[...]

quoted

w.r.t sharing infrastructure with fanotify, I only looked briefly at
your patches
and I have only a vague familiarity with landlock, so I cannot yet form an
opinion whether this is a good idea, but I wanted to give you a few more
data points about fanotify that seem relevant.

1. There is already some intersection of fanotify and audit lsm via the
fanotify_response_info_audit_rule extension for permission
events, so it's kind of a precedent of using fanotify to aid an lsm

2. See this fan_pre_modify-wip branch [1] and specifically commit
   "fanotify: introduce directory entry pre-modify permission events"
I do have an intention to add create/delete/rename permission events.
Note that the new fsnotify hooks are added in to do_ vfs helpers, not very
far from the security_path_ lsm hooks, but not exactly in the same place
because we want to fsnotify hooks to be before taking vfs locks, to allow
listener to write to filesystem from event context.
There are different semantics than just ALLOW/DENY that you need,
therefore, only if we move the security_path_ hooks outside the
vfs locks, our use cases could use the same hooks

Hi Amir,

(this is a slightly long message - feel free to respond at your convenience,
thank you in advance!)

Thanks a lot for mentioning this branch, and for the explanation! I've had a
look and realized that the changes you have there will be very useful for
this patch, and in fact, I've already tried a worse attempt of this (not
included in this patch series yet) to create some security_pathname_ hooks
that takes the parent struct path + last name as char*, that will be called
before locking the parent.  (We can't have an unprivileged supervisor cause
a directory to be locked indefinitely, which will also block users outside
of the landlock domain)

I'm not sure if we can move security_path tho, because it takes the dentry
of the child as an argument, and (I think at least for create / mknod /
link) that dentry is only created after locking.  Hence the proposal for
separate security_pathname_ hooks.  A search shows that currently AppArmor
and TOMOYO (plus Landlock) uses the security_path_ hooks that would need
changing, if we move it (and we will have to understand if the move is ok to
do for the other two LSMs...)

However, I think it would still make a lot of sense to align with fsnotify
here, as you have already made the changes that I would need to do anyway
should I implement the proposed new hooks.  I think a sensible thing might
be to have the extra LSM hooks be called alongside fsnotify_(re)name_perm -
following the pattern of what currently happens with fsnotify_open_perm
(i.e. security_file_open called first, then fsnotify_open_perm right after).

Yes, I think it would make sense to use the same hooks for fanotify and
other security subsystems, or at least to share them.  It would improve
consistency across different Linux subsystems and simplify changes and
maintenance where these hooks are called.

What's your thought on this? Do you think it would be a good idea to have
LSM hook equivalents of the fsnotify (re)name perm hooks / fanotify
pre-modify events?

Also, do you have a rough estimate of when you would upstream the
fa/fsnotify changes? (asking just to get an idea of things, not trying to
rush or anything :) I suspect this supervise patch would take a while
anyway)

If you think the general idea is right, here are some further questions I
have:

I think going by this approach any error return from security_pathname_mknod
(or in fact, fsnotify_name_perm) when called in the open O_CREAT code path
would end up becoming a -EROFS.  Can we turn the bool got_write in
open_last_lookups into an int to store any error from mnt_want_write_parent,
and return it if lookup_open returns -EROFS?  This is so that the user space
still gets an -EACCESS on create denials by landlock (and in fact, if
fanotify denies a create maybe we want it to return the correct errno
also?). Maybe there is a better way, this is just my first though...

I also noticed that you don't currently have fsnotify hook calls for link
(although it does end up invoking the name_perm hook on the dest with
MAY_CREATE).  I want to propose also changing do_linkat to (pass the right
flags to filename_create_srcu -> mnt_want_write_parent to) call the
security_pathname_link hook (instead of the LSM hook it would normally call
for a creation event in this proposal) that is basically like
security_path_link, except passing the destination as a dir/name pair, and
without holding vfs lock (still passing in the dentry of the source itself),
to enable landlock to handle link requests separately. Do you think this is
alright?  (Maybe the code would be a bit convoluted if written verbatim from
this logic, maybe there is a better way, but the general idea is hopefully
right)

btw, side question, I see that you added srcu read sections around the
events - I'm not familiar with rcu/locking usage in vfs but is this for
preventing e.g. changing the mount in some way (but still allowing access /
changes to the directory)?

I realize I'm asking you a lot of things - big thanks in advance!  (also let
me know if I should be pulling in other VFS maintainers)

--

For Mickaël,

Would you be on board with changing Landlock to use the new hooks as
mentioned above?  My thinking is that it shouldn't make any difference in
terms of security - Landlock permissions for e.g. creating/deleting files
are based on the parent, and in fact except for link and rename, the
hook_path_ functions in Landlock don't even use the dentry argument.  If
you're happy with the general direction of this, I can investigate further
and test it out etc.  This change might also reduce the impact of Landlock
on non-landlocked processes, if we avoid holding exclusive inode lock while
evaluating rules / traversing paths...? (Just a thought, not measured)

This looks reasonable.  As long as the semantic does not change it
should be good and Landlock tests should pass.  That would also require
other users of this hook to make sure it works for them too.  If it is
not the case, I guess we could add an alternative hooks with different
properties.  However, see the issue and the alternative approach below.

In terms of other aspects, ignoring supervisors for now, moving to these
hooks:

- Should make no difference in the "happy" (access allowed) case

- Only when an access is disallowed, in order to know what error to
  return, we can check (within Landlock hook handler) if the target
  already exists - if yes, return -EEXIST, otherwise -EACCESS

We should avoid as much as possible to reimplement the error types in
fanotify/LSM hooks.  This is partially done for the VFS, and completely
duplicated for the network, which can lead to inconsistent errors.  It
would be good to only have one source of truth, but that might not be
possible in all cases.

If this is too large of a change at this point and you see / would prefer
another way we can progress this series (at least the initial version), let
me know.

For this patch series to work, we need all (used) LSM hooks to be
blockable (and interruptible).  We should then investigate if this is
possible, especially with the new fanotify hooks, but I don't think it
would work for all hooks (already or that will potentially be used by
Landlock).

An alternative approach would be to add a task_work (executed before
returning to user space) that will wait for the supervisor to take a
decision, and in the meantime the LSM hook would return -ERESTARTNOINTR
for the syscall to start again after the wait.  However, because the
request to the supervisor would be called outside of the hook, it should
not be possible to directly allow the request (because of race
condition) but to update the domain accordingly.  The restarted syscall
must not trigger a supervisor request though.

Kind regards,
Tingmao

quoted

3. There is a recent attempt to add BPF filter to fanotify [2]
which is driven among other things from the long standing requirement
to add subtree filtering to fanotify watches.
The challenge with all the attempt to implement a subtree filter so far,
is that adding vfs performance overhead for all the users in the system
is unacceptable.

IIUC, landlock rule set can already express a subtree filter (?),
so it is intriguing to know if there is room for some integration on this
aspect, but my guess is that landlock mostly uses subtree filter
after filtering by specific pids (?), so it can avoid the performance
overhead of a subtree filter on most of the users in the system.

Hope this information is useful.

Thanks,
Amir.

[1] https://github.com/amir73il/linux/commits/fan_pre_modify-wip/
[2] https://lore.kernel.org/linux-fsdevel/20241122225958.1775625-1-song@kernel.org/ (local)