On 3/11/25 20:58, Song Liu wrote:
On Tue, Mar 11, 2025 at 12:28 PM Mickaël Salaün <mic@xxxxxxxxxxx> wrote:
On Tue, Mar 11, 2025 at 12:42:05AM +0000, Tingmao Wang wrote:
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).
I think there is a fundamental difference between LSM hooks and fsnotify,
so putting fsnotify behind some LSM hooks might be weird. Specifically,
LSM hooks are always global. If a LSM attaches to a hook, say
security_file_open, it will see all the file open calls in the system. On the
other hand, each fsnotify rule only applies to a group, so that one fanotify
handler doesn't touch files watched by another fanotify handler. Given this
difference, I am not sure how fsnotify LSM hooks should look like.
Does this make sense?
To clarify, I wasn't suggesting that we put one hook _behind_ another
("behind" in the sense of one calling the other), just that the place
that calls the new fsnotify_name_perm/fsnotify_rename_perm hook (in
Amir's WIP branch) could also be made to call some new LSM hooks in
addition to fsnotify (i.e. security_pathname_create/delete/rename).
My understanding of the current code is that VFS calls security_... and
fsnotify_... unconditionally, and the fsnotify_... functions figure out
who needs to be notified.
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.
Mickaël - I'm not sure what you mean by "the same hook" - do you mean
the relevant VFS functions could call both fsnotify and LSM hooks?
[...]
--
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)
I think the filter for process/thread is usually faster than the filter for
file/path/subtree? Therefore, it is better for landlock to check the filter for
process/thread first. Did I miss/misunderstand something?
Sorry, I should have clarified that the "impact" I'm talking about here
isn't referring to directly the time it takes for landlock to decide if
an access is allowed or not - in a non-landlocked process, the landlock
hooks already returns really early and fast. However, I'm thinking of a
situation where a landlocked process makes lots of create/delete etc
requests on a directory, and landlock does need to do some work (e.g.
path traversal) to decide those access. Because the
security_path_mknod/unlink/... hooks are called in the VFS from a place
where it is holding an exclusive lock on the directory (for O_CREAT'ing
a child or other directory modification cases), when landlock is working
out an access by the landlocked process, no other tasks will be able to
read/write the directory (they will be blocked on the lock), even if
their access have nothing to do with landlock.
I should add that this is probably just a very minor impact: the user
space can't cause the dir to be blocked for arbitrary amount of time, at
worst slowing everyone else down by a bit if it deliberately creates
lots of layers (max 16) each with lots of rules (the ruleset evaluation
is O(log(#rules) * dir_depth)). I didn't measure it, it's just something
that occurred to me that could be improved by using new hooks that
aren't called with inode locks held.
Kind regards,
Tingmao
Thanks,
Song
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.
