On 10/20/22 2:41 PM, Andrii Nakryiko wrote:
> 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;
>
> } ?
>
Oops! Originally the goto was in an else clause.
>
> 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
>>
