On Fri, Jan 5, 2024 at 1:45 PM Linus Torvalds
<torvalds@xxxxxxxxxxxxxxxxxxx> wrote:
>
> Ok, I've gone through the whole series now, and I don't find anything
> objectionable.
That's great, thanks for reviewing!
>
> Which may only mean that I didn't notice something, of course, but at
> least there's nothing I'd consider obvious.
>
> I keep coming back to this 03/29 patch, because it's kind of the heart
> of it, and I have one more small nit, but it's also purely stylistic:
>
> On Wed, 3 Jan 2024 at 14:21, Andrii Nakryiko <andrii@xxxxxxxxxx> wrote:
> >
> > +bool bpf_token_capable(const struct bpf_token *token, int cap)
> > +{
> > + /* BPF token allows ns_capable() level of capabilities, but only if
> > + * token's userns is *exactly* the same as current user's userns
> > + */
> > + if (token && current_user_ns() == token->userns) {
> > + if (ns_capable(token->userns, cap))
> > + return true;
> > + if (cap != CAP_SYS_ADMIN && ns_capable(token->userns, CAP_SYS_ADMIN))
> > + return true;
> > + }
> > + /* otherwise fallback to capable() checks */
> > + return capable(cap) || (cap != CAP_SYS_ADMIN && capable(CAP_SYS_ADMIN));
> > +}
>
> This *feels* like it should be written as
>
> bool bpf_token_capable(const struct bpf_token *token, int cap)
> {
> struct user_namespace *ns = &init_ns;
>
> /* BPF token allows ns_capable() level of capabilities, but only if
> * token's userns is *exactly* the same as current user's userns
> */
> if (token && current_user_ns() == token->userns)
> ns = token->userns;
> return ns_capable(ns, cap) ||
> (cap != CAP_SYS_ADMIN && capable(CAP_SYS_ADMIN));
> }
>
> And yes, I realize that the function will end up later growing a
>
> security_bpf_token_capable(token, cap)
>
> test inside that 'if (token ..)' statement, and this would change the
> order of that test so that the LSM hook would now be done before the
> capability checks are done, but that all still seems just more of an
> argument for the simplification.
>
> So the end result would be something like
>
> bool bpf_token_capable(const struct bpf_token *token, int cap)
> {
> struct user_namespace *ns = &init_ns;
>
> if (token && current_user_ns() == token->userns) {
> if (security_bpf_token_capable(token, cap) < 0)
> return false;
> ns = token->userns;
> }
> return ns_capable(ns, cap) ||
> (cap != CAP_SYS_ADMIN && capable(CAP_SYS_ADMIN));
> }
Yep, it makes sense to use ns_capable with init_ns. I'll change those
two patches to end up with something like what you suggested here.
>
> although I feel that with that LSM hook, maybe this all should return
> the error code (zero or negative), not a bool for success?
>
> Also, should "current_user_ns() != token->userns" perhaps be an error
> condition, rather than a "fall back to init_ns" condition?
>
> Again, none of this is a big deal. I do think you're dropping the LSM
> error code on the floor, and are duplicating the "ns_capable()" vs
> "capable()" logic as-is, but none of this is a deal breaker, just more
> of my commentary on the patch and about the logic here.
>
> And yeah, I don't exactly love how you say "ok, if there's a token and
> it doesn't match, I'll not use it" rather than "if the token namespace
> doesn't match, it's an error", but maybe there's some usability issue
> here?
Yes, usability was the primary concern. The overall idea with BPF
token is to make most BPF applications not care or even potentially
know about its existence, and mostly leave it up to administrators
and/or container managers to set up an environment with BPF token
delegation. To make that all possible, libbpf will opportunistically
try to create BPF token from BPF FS in the container (typically
/sys/fs/bpf, but it can be tuned, of course). And so if BPF token can
actually prevent, say, BPF program loading, because it didn't allow
particular program type to be loaded or whatnot, that would be a
regression of behavior relative to if BPF token was never even used.
So I consciously wanted a behavior in which BPF token can be used as a
sort of potential/additional rights, but otherwise just fallback to
current behavior based on capable(CAP_BPF) and other caps we use.
The alternative to the above would be creating a few more APIs to
proactively check if a given BPF token instance would allow whatever
operation libbpf needs to perform, and if not, not using it. Which
would be used to achieve the exact same behavior but in a more round
about way.
And the last piece of thinking was that if the user actually would
want to fail bpf() operation if the BPF token doesn't grant such
permissions, we can add a flag that would force this behavior. Some
sort of BPF_F_TOKEN_STRICT that can be optionally specified. But I
wanted to wait for an actual production use case that would want that
(I'm not aware of any right now).
>
> Linus
