On Tue, Jul 23, 2019 at 7:17 PM Aaron Goidel <acgoide@xxxxxxxxxxxxx> wrote:
>
> On 7/18/19 12:16 PM, Amir Goldstein wrote:
> > On Thu, Jul 18, 2019 at 5:31 PM Aaron Goidel <acgoide@xxxxxxxxxxxxx> 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.
