Re: [PATCH] fanotify: move path permission and security check

From: Amir Goldstein <amir73il@gmail.com>
To: Mete Durlu <meted@linux.ibm.com>
Cc: jack@suse.cz, repnop@google.com, linux-fsdevel@vger.kernel.org
Subject: Re: [PATCH] fanotify: move path permission and security check
Date: Fri, 1 Mar 2024 11:52:16 +0200	[thread overview]
Message-ID: <CAOQ4uxh+Od_+ZuLDorbFw6nOnsuabOreH4OE=uP_JE53f0rotA@mail.gmail.com> (raw)
In-Reply-To: <20240229174145.3405638-1-meted@linux.ibm.com>

On Thu, Feb 29, 2024 at 7:53 PM Mete Durlu <meted@linux.ibm.com> wrote:
>
> In current state do_fanotify_mark() does path permission and security
> checking before doing the event configuration checks. In the case
> where user configures mount and sb marks with kernel internal pseudo
> fs, security_path_notify() yields an EACESS and causes an earlier
> exit. Instead, this particular case should have been handled by
> fanotify_events_supported() and exited with an EINVAL.

What makes you say that this is the expected outcome?
I'd say that the expected outcome is undefined and we have no reason
to commit to either  EACCESS or EINVAL outcome.

I don't really mind the change of outcome, but to me it seems
nicer that those tests are inside fanotify_find_path(), so I will
want to get a good reason for moving them out.

> Move path perm and security checks under the event validation to
> prevent this from happening.
> Simple reproducer;
>
>         fan_d = fanotify_init(FAN_CLASS_NOTIF, O_RDONLY);
>         pipe2(pipes, O_CLOEXEC);
>         fanotify_mark(fan_d,
>                       FAN_MARK_ADD |
>                       FAN_MARK_MOUNT,
>                       FAN_ACCESS,
>                       pipes[0],
>                       NULL);
>         // expected: EINVAL (22), produces: EACCES (13)
>         printf("mark errno: %d\n", errno);
>
> Another reproducer;
> ltp/testcases/kernel/syscalls/fanotify/fanotify14
>
> Fixes: 69562eb0bd3e ("fanotify: disallow mount/sb marks on kernel internal pseudo fs")
>
> Signed-off-by: Mete Durlu <meted@linux.ibm.com>
> ---
>  fs/notify/fanotify/fanotify_user.c | 24 +++++++++---------------
>  1 file changed, 9 insertions(+), 15 deletions(-)
>
> diff --git a/fs/notify/fanotify/fanotify_user.c b/fs/notify/fanotify/fanotify_user.c
> index fbdc63cc10d9..14121ad0e10d 100644
> --- a/fs/notify/fanotify/fanotify_user.c
> +++ b/fs/notify/fanotify/fanotify_user.c
> @@ -1015,7 +1015,7 @@ static int fanotify_find_path(int dfd, const char __user *filename,
>                         fdput(f);
>                         goto out;
>                 }
> -
> +               ret = 0;

Better convert all gotos in this helper to return.
There is nothing in the out label.

>                 *path = f.file->f_path;
>                 path_get(path);
>                 fdput(f);
> @@ -1028,21 +1028,7 @@ static int fanotify_find_path(int dfd, const char __user *filename,
>                         lookup_flags |= LOOKUP_DIRECTORY;
>
>                 ret = user_path_at(dfd, filename, lookup_flags, path);
> -               if (ret)
> -                       goto out;
>         }
> -
> -       /* you can only watch an inode if you have read permissions on it */
> -       ret = path_permission(path, MAY_READ);
> -       if (ret) {
> -               path_put(path);
> -               goto out;
> -       }
> -
> -       ret = security_path_notify(path, mask, obj_type);
> -       if (ret)
> -               path_put(path);
> -
>  out:
>         return ret;
>  }
> @@ -1894,6 +1880,14 @@ static int do_fanotify_mark(int fanotify_fd, unsigned int flags, __u64 mask,
>                 if (ret)
>                         goto path_put_and_out;
>         }
> +       /* you can only watch an inode if you have read permissions on it */
> +       ret = path_permission(&path, MAY_READ);
> +       if (ret)
> +               goto path_put_and_out;
> +
> +       ret = security_path_notify(&path, mask, obj_type);
> +       if (ret)
> +               goto path_put_and_out;
>
>         if (fid_mode) {
>                 ret = fanotify_test_fsid(path.dentry, flags, &__fsid);

If we do accept your argument that security_path_notify() should be
after fanotify_events_supported(). Why not also after fanotify_test_fsid()
and fanotify_test_fid()?

The suggested change of behavior seems arbitrary to me.

Thanks,
Amir.