Re: [Non-DoD Source] Re: [PATCH] fanotify, inotify, dnotify, security: add security hook for fs notifications

[Amir Goldstein <amir73il@gmail.com>]

> >>> +       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.

> >>> +
> >>> +       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.

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