[RFC PATCH 0/9] Landlock supervise: a mechanism for interactive permission requests

[Tingmao Wang]

On 3/6/25 17:07, Amir Goldstein wrote:
[...]
> 
> 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).

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)

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

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.

Kind regards,
Tingmao

> 
> 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/