Re: [PATCH] fuse: Rearrange fuse_allow_current_process checks

[Date Prev][Date Next][Thread Prev][Thread Next][Date Index][Thread Index]

 



On Thu, Oct 20, 2022 at 11:39 AM Dave Marchevsky <davemarchevsky@xxxxxx> wrote:
>
> This is a followup to a previous commit of mine [0], which added the
> allow_sys_admin_access && capable(CAP_SYS_ADMIN) check. This patch
> rearranges the order of checks in fuse_allow_current_process without
> changing functionality.
>
> [0] added allow_sys_admin_access && capable(CAP_SYS_ADMIN) check to the
> beginning of the function, with the reasoning that
> allow_sys_admin_access should be an 'escape hatch' for users with
> CAP_SYS_ADMIN, allowing them to skip any subsequent checks.
>
> However, placing this new check first results in many capable() calls when
> allow_sys_admin_access is set, where another check would've also
> returned 1. This can be problematic when a BPF program is tracing
> capable() calls.
>
> At Meta we ran into such a scenario recently. On a host where
> allow_sys_admin_access is set but most of the FUSE access is from
> processes which would pass other checks - i.e. they don't need
> CAP_SYS_ADMIN 'escape hatch' - this results in an unnecessary capable()
> call for each fs op. We also have a daemon tracing capable() with BPF and
> doing some data collection, so tracing these extraneous capable() calls
> has the potential to regress performance for an application doing many
> FUSE ops.
>
> So rearrange the order of these checks such that CAP_SYS_ADMIN 'escape
> hatch' is checked last. Previously, if allow_other is set on the
> fuse_conn, uid/gid checking doesn't happen as current_in_userns result
> is returned. It's necessary to add a 'goto' here to skip past uid/gid
> check to maintain those semantics here.
>
>   [0]: commit 9ccf47b26b73 ("fuse: Add module param for CAP_SYS_ADMIN access bypassing allow_other")
>
> Signed-off-by: Dave Marchevsky <davemarchevsky@xxxxxx>
> Suggested-by: Andrii Nakryiko <andrii@xxxxxxxxxx>
> Cc: Christian Brauner <brauner@xxxxxxxxxx>
> ---
>  fs/fuse/dir.c | 11 +++++++----
>  1 file changed, 7 insertions(+), 4 deletions(-)
>
> diff --git a/fs/fuse/dir.c b/fs/fuse/dir.c
> index 2c4b08a6ec81..070e1beba838 100644
> --- a/fs/fuse/dir.c
> +++ b/fs/fuse/dir.c
> @@ -1254,11 +1254,10 @@ int fuse_allow_current_process(struct fuse_conn *fc)
>  {
>         const struct cred *cred;
>
> -       if (allow_sys_admin_access && capable(CAP_SYS_ADMIN))
> -               return 1;
> -
>         if (fc->allow_other)

{

> -               return current_in_userns(fc->user_ns);
> +               if (current_in_userns(fc->user_ns))
> +                       return 1;
> +               goto skip_cred_check;

} ?


Otherwise, makes sense, thanks!

>
>         cred = current_cred();
>         if (uid_eq(cred->euid, fc->user_id) &&
> @@ -1269,6 +1268,10 @@ int fuse_allow_current_process(struct fuse_conn *fc)
>             gid_eq(cred->gid,  fc->group_id))
>                 return 1;
>
> +skip_cred_check:
> +       if (allow_sys_admin_access && capable(CAP_SYS_ADMIN))
> +               return 1;
> +
>         return 0;
>  }
>
> --
> 2.30.2
>



[Index of Archives]     [Linux Ext4 Filesystem]     [Union Filesystem]     [Filesystem Testing]     [Ceph Users]     [Ecryptfs]     [NTFS 3]     [AutoFS]     [Kernel Newbies]     [Share Photos]     [Security]     [Netfilter]     [Bugtraq]     [Yosemite News]     [MIPS Linux]     [ARM Linux]     [Linux Security]     [Linux Cachefs]     [Reiser Filesystem]     [Linux RAID]     [NTFS 3]     [Samba]     [Device Mapper]     [CEPH Development]

  Powered by Linux