On Mon, Mar 10, 2025 at 12:39:17AM +0000, Tingmao Wang wrote: > On 3/4/25 19:50, Mickaël Salaün wrote: > > On Tue, Mar 04, 2025 at 01:13:02AM +0000, Tingmao Wang wrote: > > > NOTE from future me: This implementation which waits for user response > > > while blocking inside the current security_path_* hooks is problematic due > > > to taking exclusive inode lock on the parent directory, and while I have a > > > proposal for a solution, outlined below, I haven't managed to include the > > > code for that in this version of the patch. Thus for this commit in > > > particular I'm probably more looking for suggestions on the approach > > > rather than code review. Please see the TODO section at the end of this > > > message before reviewing this patch. > > > > This is good for an RFC. > > > > > > > > ---- > > > > > > This patch implements a proof-of-concept for modifying the current > > > landlock LSM hooks to send supervisor events and wait for responses, when > > > a supervised layer is involved. > > > > > > In this design, access requests which would end up being denied by other > > > non-supervised landlock layers (or which would fail the normal inode > > > permission check anyways - but this is currently TODO, I only thought of > > > this afterwards) are denied straight away to avoid pointless supervisor > > > notifications. > > > > Yes, only denied access should be forwarded to the supervisor. > > I assume you meant only denied access *by the supervised layers* should be > forwarded to the supervisor. Yes > > > In another patch series we could enable the supervisor to update its layer > > with new rules as well. > > I did consider the possibility of this - if the supervisor has decided to > allow all future access to e.g. a directory, ideally this can be "offloaded" > to the kernel, but I was a bit worried about the fact that landlock > currently quite heavily assumes the domain is immutable. While in the > supervised case breaking that rule here should be alright (no worse > security), not sure if there is some potential logic / data race bugs if we > now make domains mutable. Domains are currently immutable, it would be good to keep this property as much as possible, but at the same time I don't see how this supervisor feature would work in practice without the ability to update the domain. > > > > > The audit patch series should help to properly identify which layer > > denied a request, and to only use the related supervisor. > > The current patch does correctly identify which layer(s) (and sends events > to the right supervisor(s)), but aligning with and re-using code in the > audit patch is sensible. Will have a look. Yes please, some helpers look very similar. It would be useful if you reviewed this part in the audit patch series. > > > > > > > > > Currently current_check_access_path only gets the path of the parent > > > directory for create/remove operations, which is not enough for what we > > > want to pass to the supervisor. Therefore we extend it by passing in any > > > relevant child dentry (but see TODO below - this may not be possible with > > > the proper implementation). > > > > Hmm, I'm not sure this kind of information is required (this is not > > implemented for the audit support). The supervisor should be fine > > getting only which access is missing, right? > > > > > > > > This initial implementation doesn't handle links and renames, and for now > > > these operations behave as if no supervisor is present (and thus will be > > > denied, unless it is allowed by the layer rules). Also note that we can > > > get spurious create requests if the program tries to O_CREAT open an > > > existing file that exists but not in the dcache (from my understanding). > > > > > > Event IDs (referred to as an opaque cookie in the uapi) are currently > > > generated with a simple `next_event_id++`. I considered using e.g. xarray > > > but decided to not for this PoC. Suggestions welcome. (Note that we have > > > to design our own event id even if we use an extension of fanotify, as > > > fanotify uses a file descriptor to identify events, which is not generic > > > enough for us) > > > > That's another noticable difference with fanotify. You can add it to > > the next cover letter. > > > > > > > > ---- > > > > > > TODO: > > > > > > When testing this I realized that doing it this way means that for the > > > create/delete case, we end up holding an exclusive inode lock on the > > > parent directory while waiting for supervisor to respond (see namei.c - > > > security_path_mknod is called in may_o_create <- lookup_open which has an > > > exclusive lock if O_CREAT is passed), which will prevent all other tasks > > > from accessing that directory (regardless of whether or not they are under > > > landlock). > > > > Could we use a landlock_object to identify this inode instead? > > Sorry - earlier when reading this I didn't quite understand this suggestion > and forgot to say so, however the problem here is the location of the > security_path_... hooks (by the time they are called the lock is already > held). I'm not sure how we identify the inode makes a difference? Yes, we should just be able to create a O_PATH FD from the hooks, but in the task_work (see my other reply). > > > > > > > > > This is clearly unacceptable, but since landlock (and also this extension) > > > doesn't actually need a dentry for the child (which is allocated after the > > > inode lock), I think this is not unsolvable. I'm experimenting with > > > creating a new LSM hook, something like security_pathname_mknod > > > (suggestions welcome), which will be called after we looked up the dentry > > > for the parent (to prevent racing symlinks TOCTOU), but before we take the > > > lock for it. Such a hook can still take as argument the parent dentry, > > > plus name of the child (instead of a struct path for it). > > > > > > Suggestions for alternative approaches are definitely welcome! > > > > > > Signed-off-by: Tingmao Wang <m@xxxxxxxxxx> > > > --- > > > security/landlock/fs.c | 134 ++++++++++++++++++++++++++++++++-- > > > security/landlock/supervise.c | 122 +++++++++++++++++++++++++++++++ > > > security/landlock/supervise.h | 106 ++++++++++++++++++++++++++- > > > 3 files changed, 354 insertions(+), 8 deletions(-) > > > > > > > [...] > >
