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? > 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. [...] > > -- > > > > 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? 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. >
