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