Received-SPF: pass (google.com: best guess record for domain of linux-kernel-owner@vger.kernel.org designates 209.132.180.67 as permitted sender) client-ip=209.132.180.67;
MIME-Version: 1.0
References: <20190731153443.4984-1-acgoide@tycho.nsa.gov> <CAHC9VhQUoDwBiLi+BiW=_Px18v3xMhhGYDD2mLdu9YZJDWw1yg@mail.gmail.com>
 <CAOQ4uxigYZunXgq0BubRFNM51Kh_g3wrtyNH77PozUX+3sM=aQ@mail.gmail.com> <e69f95ba-3da7-380a-ef14-cc866172d79a@tycho.nsa.gov>
In-Reply-To: <e69f95ba-3da7-380a-ef14-cc866172d79a@tycho.nsa.gov>
From:   Amir Goldstein <amir73il@gmail.com>
Date:   Fri, 9 Aug 2019 19:43:09 +0300
Message-ID: <CAOQ4uxh4+SDCF7HHwSxGFx01ZyJ43VSLhLM2dDFY2AQ0HkkuvA@mail.gmail.com>
Subject: Re: [Non-DoD Source] Re: [PATCH] fanotify, inotify, dnotify,
 security: add security hook for fs notifications
To:     Aaron Goidel <acgoide@tycho.nsa.gov>
Cc:     Paul Moore <paul@paul-moore.com>, selinux@vger.kernel.org,
        LSM List <linux-security-module@vger.kernel.org>,
        linux-fsdevel <linux-fsdevel@vger.kernel.org>,
        David Howells <dhowells@redhat.com>, Jan Kara <jack@suse.cz>,
        James Morris <jmorris@namei.org>,
        Stephen Smalley <sds@tycho.nsa.gov>,
        linux-kernel <linux-kernel@vger.kernel.org>
Content-Type: text/plain; charset="UTF-8"
Sender: linux-kernel-owner@vger.kernel.org
Precedence: bulk

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