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: <20190718143042.11059-1-acgoide@tycho.nsa.gov> <CAOQ4uxjCR76nbV_Lmoegaq6NqovWZD-XWEVS-X3e=BtDdjKkXQ@mail.gmail.com>
 <c74ad814-f188-37c6-9b3a-51178b538a2b@tycho.nsa.gov>
In-Reply-To: <c74ad814-f188-37c6-9b3a-51178b538a2b@tycho.nsa.gov>
From:   Amir Goldstein <amir73il@gmail.com>
Date:   Tue, 23 Jul 2019 21:49:32 +0300
Message-ID: <CAOQ4uxjzwCjOsiwE6DuaXNvLQN2QKRyOc6Tkw-xFZohUZSMtNw@mail.gmail.com>
Subject: Re: [Non-DoD Source] Re: [RFC PATCH v2] 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

On Tue, Jul 23, 2019 at 7:17 PM Aaron Goidel <acgoide@tycho.nsa.gov> wrote:
>
> On 7/18/19 12:16 PM, Amir Goldstein wrote:
> > On Thu, Jul 18, 2019 at 5:31 PM Aaron Goidel <acgoide@tycho.nsa.gov> wrote:
> >>
> >> diff --git a/fs/notify/fanotify/fanotify_user.c b/fs/notify/fanotify/fanotify_user.c
> >> index a90bb19dcfa2..9e3137badb6b 100644
> >> --- a/fs/notify/fanotify/fanotify_user.c
> >> +++ b/fs/notify/fanotify/fanotify_user.c
> >> @@ -528,9 +528,10 @@ static const struct file_operations fanotify_fops = {
> >>   };
> >>
> >>   static int fanotify_find_path(int dfd, const char __user *filename,
> >> -                             struct path *path, unsigned int flags)
> >> +                             struct path *path, unsigned int flags, __u64 mask)
> >>   {
> >>          int ret;
> >> +       unsigned int mark_type;
> >>
> >>          pr_debug("%s: dfd=%d filename=%p flags=%x\n", __func__,
> >>                   dfd, filename, flags);
> >> @@ -567,8 +568,30 @@ static int fanotify_find_path(int dfd, const char __user *filename,
> >>
> >>          /* you can only watch an inode if you have read permissions on it */
> >>          ret = inode_permission(path->dentry->d_inode, MAY_READ);
> >> +       if (ret) {
> >> +               path_put(path);
> >> +               goto out;
> >> +       }
> >> +
> >> +       switch (flags & FANOTIFY_MARK_TYPE_BITS) {
> >> +       case FAN_MARK_MOUNT:
> >> +               mark_type = FSNOTIFY_OBJ_TYPE_VFSMOUNT;
> >> +               break;
> >> +       case FAN_MARK_FILESYSTEM:
> >> +               mark_type = FSNOTIFY_OBJ_TYPE_SB;
> >> +               break;
> >> +       case FAN_MARK_INODE:
> >> +               mark_type = FSNOTIFY_OBJ_TYPE_INODE;
> >> +               break;
> >> +       default:
> >> +               ret = -EINVAL;
> >> +               goto out;
> >> +       }
> >> +
> >> +       ret = security_inode_notify(path->dentry->d_inode, mask, mark_type);
> >
> > If you prefer 3 hooks security_{inode,mount,sb}_notify()
> > please place them in fanotify_add_{inode,mount,sb}_mark().
> >
> > If you prefer single hook with path argument, please pass path
> > down to fanotify_add_mark() and call security_path_notify() from there,
> > where you already have the object type argument.
> >
> I'm not clear on why you want me to move the hook call down to
> fanotify_add_mark(). I'd prefer to keep it adjacent to the existing
> inode_permission() call so that all the security checking occurs from
> one place.

Fine.

> Moving it down requires adding a path arg to that entire call
> chain, even though it wouldn't otherwise be needed.

That doesn't matter.

> And that raises the
> question of whether to continue passing the mnt_sb, mnt, or inode
> separately or just extract all those from the path inside of
> fanotify_add_*_mark().

You lost me. The major issue I have is with passing @inode argument
to hook for adding a mount watch. Makes no sense to me as @inode
may be accessed from any mount and without passing @path to hook
this information is lost.

>
> It also seems to destroy the parallelism with fanotify_remove_*_mark().

I don't know what that means.

> I also don't see any real benefit in splitting into three separate
> hooks, especially as some security modules will want the path or inode
> even for the mount or superblock cases, since they may have no relevant
> security information for vfsmounts or superblocks.

OK. that is an argument for single hook with @path argument.
That is fine by me.

Thanks,
Amir.