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

[Tingmao Wang <m@maowtm.org>]

On 3/11/25 20:58, Song Liu wrote:
> On Tue, Mar 11, 2025 at 12:28 PM Mickaël Salaün <mic@digikod.net> 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.
>>