quoted

quoted

quoted

+       switch (flags & FANOTIFY_MARK_TYPE_BITS) {
+       case FAN_MARK_MOUNT:
+               obj_type = FSNOTIFY_OBJ_TYPE_VFSMOUNT;
+               break;
+       case FAN_MARK_FILESYSTEM:
+               obj_type = FSNOTIFY_OBJ_TYPE_SB;
+               break;
+       case FAN_MARK_INODE:
+               obj_type = FSNOTIFY_OBJ_TYPE_INODE;
+               break;
+       default:
+               ret = -EINVAL;
+               goto out;
+       }

Sorry, I just can't stand this extra switch statement here.
Please initialize obj_type at the very first switch statement in
do_fanotify_mark() and pass it to fanotify_find_path().
Preferably also make it a helper that returns either
valid obj_type or <0 for error.

I have no problem moving the initialization of obj_type up one level to
do_fanotify_mark(). I don't think that a helper is necessary at this
juncture as this logic seems to only exist in one place. Should this
change, then there would be merit to having a separate function.

Ok.

quoted

quoted

quoted

+
+       ret = security_path_notify(path, mask, obj_type);
         if (ret)
                 path_put(path);

It is probably best to mask out FANOTIFY_EVENT_FLAGS
when calling the hook. Although FAN_EVENT_ON_CHILD
and FAN_ONDIR do map to corresponding FS_ constants,
the security hooks from dnotify and inotify do not pass these
flags, so the security module cannot use them as reliable
information, so it will have to assume that they have been
requested anyway.

Alternatively, make sure that dnotify/inotify security hooks
always set these two flags, by fixing up and using the
dnotify/inotify arg_to_mask conversion helpers before calling
the security hook.

I think that at this point either approach you mentioned makes just as
much sense. If it's all the same to you, Amir, I'll just change the
caller in fanotify to include (mask) & ~(FANOTIFY_EVENT_FLAGS)

On second look, let's go with (mask & ALL_FSNOTIFY_EVENTS)
It seems simpler and more appropriate way to convert to FS_ flags.

[...]

quoted

quoted

quoted

-       ret = inotify_find_inode(pathname, &path, flags);
+       ret = inotify_find_inode(pathname, &path, flags, mask);

Please use (mask & IN_ALL_EVENTS) for converting to common FS_ flags
or use the inotify_arg_to_mask() conversion helper, which contains more
details irrelevant for the security hook.
Otherwise mask may contain flags like IN_MASK_CREATE, which mean
different things on different backends and the security module cannot tell
the difference.

Also note that at this point, before inotify_arg_to_mask(), the mask does
not yet contain FS_EVENT_ON_CHILD, which could be interesting for
the security hook (fanotify users can opt-in with FAN_EVENT_ON_CHILD).
Not a big deal though as security hook can assume the worse
(that events on child are requested).

I'll use (mask & IN_ALL_EVENTS).

OK.

I can implement the changes in the ways I mentioned above. I don't see a
need for anything more in the cases you brought up since none of them
change the logic of the hook itself or would make a substantive
difference to the operation of the hook given its current implementation.

Agree. If more flags are needed for LSMs they could be added later.

Thanks,
Amir.