On Thu, Oct 20, 2022 at 1:14 PM 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>
> ---
LGTM!
Acked-by: Andrii Nakryiko <andrii@xxxxxxxxxx>
But I would also be curious to hear from Miklos or others whether
skipping uid/gid check if fc->allow_other is true wa an intentional
logic, oversight, or just doesn't matter. Because doing:
if (fc->allow_other && current_in_userns(fc->user_ns))
return 1;
/* do uid/gid check */
if (allow_sys_admin_access && capable(CAP_SYS_ADMIN))
return 1;
Would be simpler and more linear logic, but I'm not sure if there are
subtle downsides to doing it this way.
> v1 -> v2: lore.kernel.org/linux-fsdevel/20221020183830.1077143-1-davemarchevsky@xxxxxx
>
> * Add missing brackets to allow_other if statement (Andrii)
>
> fs/fuse/dir.c | 14 +++++++++-----
> 1 file changed, 9 insertions(+), 5 deletions(-)
>
> diff --git a/fs/fuse/dir.c b/fs/fuse/dir.c
> index 2c4b08a6ec81..2f14e90907a2 100644
> --- a/fs/fuse/dir.c
> +++ b/fs/fuse/dir.c
> @@ -1254,11 +1254,11 @@ 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 (fc->allow_other) {
> + if (current_in_userns(fc->user_ns))
> + return 1;
> + goto skip_cred_check;
> + }
>
> cred = current_cred();
> if (uid_eq(cred->euid, fc->user_id) &&
> @@ -1269,6 +1269,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
>
