On Fri, Oct 21, 2022 at 09:02:30AM -0700, Andrii Nakryiko wrote:
> 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
Thanks for spotting this. I missed that. I'm not opposed to gotos it
just seems a bit ugly in this case and I'd prefer sm like (again,
completely untested):
int fuse_allow_current_process(struct fuse_conn *fc)
{
const struct cred *cred;
int ret;
if (fc->allow_other)
ret = current_in_userns(fc->user_ns);
else
ret = fuse_permissible_uidgid(fc);
if (!ret && allow_sys_admin_access && capable(CAP_SYS_ADMIN))
ret = 1;
return ret;
}
but I'm not fuzzed about it.
