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. Add a small helper, fuse_permissible_uidgid, to
make the logic easier to understand. Previously, if allow_other is set
on the fuse_conn, uid/git checking doesn't happen as current_in_userns
result is returned. These semantics are maintained here:
fuse_permissible_uidgid check only happens if allow_other is not set.
[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>
---
v2 -> v3: lore.kernel.org/linux-fsdevel/20221020201409.1815316-1-davemarchevsky@xxxxxx
* Add fuse_permissible_uidgid, rearrange fuse_allow_current_process to
not use 'goto' (Christian)
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 | 33 +++++++++++++++++++--------------
1 file changed, 19 insertions(+), 14 deletions(-)
diff --git a/fs/fuse/dir.c b/fs/fuse/dir.c
index 2c4b08a6ec81..cfd857754c75 100644
--- a/fs/fuse/dir.c
+++ b/fs/fuse/dir.c
@@ -1237,6 +1237,18 @@ int fuse_reverse_inval_entry(struct fuse_conn *fc, u64 parent_nodeid,
return err;
}
+static inline bool fuse_permissible_uidgid(struct fuse_conn *fc)
+{
+ const struct cred *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));
+}
+
/*
* Calling into a user-controlled filesystem gives the filesystem
* daemon ptrace-like capabilities over the current process. This
@@ -1252,24 +1264,17 @@ 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;
+ int ret;
if (fc->allow_other)
- return current_in_userns(fc->user_ns);
+ ret = current_in_userns(fc->user_ns);
+ else
+ ret = fuse_permissible_uidgid(fc);
- 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;
+ if (!ret && allow_sys_admin_access && capable(CAP_SYS_ADMIN))
+ ret = 1;
- return 0;
+ return ret;
}
static int fuse_access(struct inode *inode, int mask)
--
2.30.2
