[PATCH 02/10] capability: add any wrappers to test for multiple caps with exactly one audit message

[Andrii Nakryiko]

On Fri, Mar 15, 2024 at 11:41 AM Jens Axboe <axboe at kernel.dk> wrote:
>
> On 3/15/24 10:45 AM, Andrii Nakryiko wrote:
> >> +/**
> >> + * ns_capable_any - Determine if the current task has one of two superior capabilities in effect
> >> + * @ns:  The usernamespace we want the capability in
> >> + * @cap1: The capabilities to be tested for first
> >> + * @cap2: The capabilities to be tested for secondly
> >> + *
> >> + * Return true if the current task has at least one of the two given superior
> >> + * capabilities currently available for use, false if not.
> >> + *
> >> + * In contrast to or'ing capable() this call will create exactly one audit
> >> + * message, either for @cap1, if it is granted or both are not permitted,
> >> + * or @cap2, if it is granted while the other one is not.
> >> + *
> >> + * The capabilities should be ordered from least to most invasive, i.e. CAP_SYS_ADMIN last.
> >> + *
> >> + * This sets PF_SUPERPRIV on the task if the capability is available on the
> >> + * assumption that it's about to be used.
> >> + */
> >> +bool ns_capable_any(struct user_namespace *ns, int cap1, int cap2)
> >> +{
> >> +       if (cap1 == cap2)
> >> +               return ns_capable(ns, cap1);
> >> +
> >> +       if (ns_capable_noauditondeny(ns, cap1))
> >> +               return true;
> >> +
> >> +       if (ns_capable_noauditondeny(ns, cap2))
> >> +               return true;
> >> +
> >> +       return ns_capable(ns, cap1);
> >
> > this will incur an extra capable() check (with all the LSMs involved,
> > etc), and so for some cases where capability is expected to not be
> > present, this will be a regression. Is there some way to not redo the
> > check, but just audit the failure? At this point we do know that cap1
> > failed before, so might as well just log that.
>
> Not sure why that's important - if it's a failure case, and any audit
> failure should be, then why would we care if that's now doing a bit of
> extra work?

Lack of capability doesn't necessarily mean "failure". E.g., in FUSE
there are at least few places where the code checks
capable(CAP_SYS_ADMIN), and based on that decides on some limit values
or extra checks. So if !capable(CAP_SYS_ADMIN), operation doesn't
necessarily fail outright, it just has some more restricted resources
or something.

Luckily in FUSE's case it's singular capable() check, so capable_any()
won't incur extra overhead. But I was just wondering if it would be
possible to avoid this with capable_any() as well, so that no one has
to do these trade-offs.

We also had cases in production of some BPF applications tracing
cap_capable() calls, so each extra triggering of it would be a bit of
added overhead, as a general rule.

Having said the above, I do like capable_any() changes (which is why I
acked BPF side of things).

>
> I say this not knowing the full picture, as I unhelpfully was only CC'ed
> on two of the patches... Please don't do that when sending patchsets.
>
> --
> Jens Axboe
>