On Sat, Oct 22, 2022 at 6:30 AM Christian Brauner <brauner@xxxxxxxxxx> wrote:
>
> 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.
Me neither as long as it allows kernel to avoid unnecessary capable()
calls :) LGTM, thanks.
