On Tue, Mar 11, 2025 at 1:42 AM Tingmao Wang <m@xxxxxxxxxx> 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).
>
> 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?
>
No clear answer but some data points:
The fanotify permission hooks (formerly fsnotify_perm) used to be inside
security_file_{open,permission} so when I started looking at dir modification
permission events I started to try using the security_path_ hooks, but as the
work progressed I found that fsnotify hooks have different
requirements (no locks).
Later, we found out that the existing fsnotify permission hooks have different
needs than the existing security hooks (for pre-content events), so after:
1cda52f1b4611 fsnotify, lsm: Decouple fsnotify from lsm
fanotify is not using any LSM hooks and not dependent on CONFIG_SECURITY.
Mentally, I do find it easy for fsnotify and security hook to be next
to each other,
unless there is a reason to do it otherwise, because from vfs POV they
are mostly
the same, but note that my branch implements the new fsnotify hooks actually as
scopes (for sb_write_srcu) and in some cases as other people have mentioned
on this thread, the security hooks need to be inside the vfs locks,
while the fsnotify
hooks need to be outside of the locks.
> 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)
>
Besides my time to work on this, these patches are waiting for some
other things.
One is that I was waiting with promoting those patches until pre-content patches
got merged and that took longer than expected and even now there may
need to be follow ups in the next cycle.
Another thing is that these patches rely on the sb_write_srcu design concept
which is pretty intrusive to vfs, so I still need to sell this to vfs people.
I am going to make another shot of an elevator pitch at LSFMM in two weeks,
If we get past this design hurdle, the rest of the work will depend on
how much time I can spend on it.
I do *want* to make the patches in time for the 2025 LTS kernel, but it may
not be a realistic goal.
One thing that helped a lot with pushing pre-content events is that Meta
already had a production use case for it.
I do not know of anyone else that requested the pre-directory-modify
hooks (besides myself), so that may make the sale a bit harder.
If there is someone out there that does need the pre-directory-modify
hooks now would be a good time to speak up.
> 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?
IIUC you mean like this:
err = mnt_want_write_parent(&nd->path, MAY_CREATE,
&res, &idx);
if (err && err != -EROFS)
return err;
got_write = !err;
/*
* do _not_ fail yet - ....
Yes, I think that is better, because the logic in the comment only
applies to 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)
I am not sure I understand your question.
fsnotify does not need to know this is a LINK and not CREATE.
I do not know what the requirements of other LSMs for those hooks,
so hard to say if it is ok to move those hooks but my guess is not ok.
>
> 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)?
>
No. this is meant to accommodate fsnotify_wait_handle_events()
(see last patch) - wait for in-flight modifications to complete without blocking
new modifications. That's the concept that I need to sell to vfs people.
Thanks,
Amir.
