On Fri, Oct 21, 2022 at 1:09 AM Christian Brauner <brauner@xxxxxxxxxx> wrote:
>
> 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;
This needs to be checked regardless of fc->allow_other, so the change
you are proposing is not equivalent to the original logic. It does
seem like a simple goto is a cleaner approach in this particular case.
> +
> + 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)
>
