On Thu, Oct 20, 2022 at 01:14:09PM -0700, Dave Marchevsky 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>
> ---
The idea sounds good.
> 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;
I think this is misnamed especially because capabilities are creds as
well. Maybe we should not use a goto even if it makes the patch a bit
bigger (_completely untested_)?:
diff --git a/fs/fuse/dir.c b/fs/fuse/dir.c
index bb97a384dc5d..3d733e0865bf 100644
--- a/fs/fuse/dir.c
+++ b/fs/fuse/dir.c
@@ -1235,6 +1235,28 @@ int fuse_reverse_inval_entry(struct fuse_conn *fc, u64 parent_nodeid,
return err;
}
+static inline bool fuse_permissible_uidgid(const struct fuse_conn *fc)
+{
+ cred = current_cred();
+ return (uid_eq(cred->euid, fc->user_id) &&
+ uid_eq(cred->suid, fc->user_id) &&
+ uid_eq(cred->uid, fc->user_id) &&
+ gid_eq(cred->egid, fc->group_id) &&
+ gid_eq(cred->sgid, fc->group_id) &&
+ gid_eq(cred->gid, fc->group_id))
+}
+
+static inline bool fuse_permissible_other(const struct fuse_conn *fc)
+{
+ if (current_in_userns(fc->user_ns))
+ return true;
+
+ if (allow_sys_admin_access && capable(CAP_SYS_ADMIN))
+ return true;
+
+ return false;
+}
+
/*
* Calling into a user-controlled filesystem gives the filesystem
* daemon ptrace-like capabilities over the current process. This
@@ -1250,24 +1272,10 @@ int fuse_reverse_inval_entry(struct fuse_conn *fc, u64 parent_nodeid,
*/
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);
-
- cred = current_cred();
- if (uid_eq(cred->euid, fc->user_id) &&
- uid_eq(cred->suid, fc->user_id) &&
- uid_eq(cred->uid, fc->user_id) &&
- gid_eq(cred->egid, fc->group_id) &&
- gid_eq(cred->sgid, fc->group_id) &&
- gid_eq(cred->gid, fc->group_id))
- return 1;
+ return fuse_permissible_other(fc);
- return 0;
+ return fuse_permissible_uidgid(fc);
}
static int fuse_access(struct inode *inode, int mask)
