On Mon, Jan 8, 2024 at 7:07 PM Andrii Nakryiko
[off-list ref] wrote:

On Mon, Jan 8, 2024 at 8:45 AM Paul Moore [off-list ref] wrote:

quoted

On Fri, Jan 5, 2024 at 4:45 PM Linus Torvalds
[off-list ref] wrote:

quoted

On Wed, 3 Jan 2024 at 14:21, Andrii Nakryiko [off-list ref] wrote:

quoted

+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.

I have no problem with rewriting things, my only ask is that we stick
with the idea of doing the capability checks before the LSM hook.  The
DAC-before-MAC (capability-before-LSM) pattern is one we try to stick
to most everywhere in the kernel and deviating from it here could
potentially result in some odd/unexpected behavior from a user
perspective.

Makes sense, Paul. With the suggested rewrite we'll get an LSM call
before we get to ns_capable() (which we avoid doing in BPF code base,
generally speaking, after someone called this out earlier). Hmm...

I guess it will be better to keep this logic as is then, I believe it
was more of a subjective stylistical nit from Linus, so it probably is
ok to keep existing code.

I didn't read Linus' reply as a mandate, more as a
this-would-be-nice-to-have, and considering the access control
ordering I would just stick with what you have (ignoring Christian's
concerns, I'm only commenting on the LSM related stuff here).

If Linus is *really* upset with how the code is written I suspect
we'll hear from him on that.

-- 
paul-moore.com