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>
---
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
