On Fri, Jan 12, 2024 at 11:17 AM Christian Brauner [off-list ref] wrote:

quoted

quoted

My point is that the capable logic will walk upwards the user namespace
hierarchy from the token->userns until the user namespace of the caller
and terminate when it reached the init_user_ns.

A caller is located in some namespace at the point where they call this
function. They provided a token. The caller isn't capable in the
namespace of the token so the function falls back to init_user_ns. Two
interesting cases:

(1) The caller wasn't in an ancestor userns of the token. If that's the
    case then it follows that the caller also wasn't in the init_user_ns
    because the init_user_ns is a descendant of all other user
    namespaces. So falling back will fail.

agreed

quoted

(2) The caller was in the same or an ancestor user namespace of the
    token but didn't have the capability in that user namespace:

     (i) They were in a non-init_user_ns. Therefore they can't be
         privileged in init_user_ns.
    (ii) They were in init_user_ns. Therefore, they lacked privileges in
         the init_user_ns.

In both cases your fallback will do nothing iiuc.

agreed as well

And I agree in general that there isn't a *practically useful* case
where this would matter much. But there is still (at least one) case
where there could be a regression: if token is created in
init_user_ns, caller has CAP_BPF in init_user_ns, caller passes that
token to BPF_PROG_LOAD, and LSM policy rejects that token in
security_bpf_token_capable(). Without the above implementation such
operation will be rejected, even though if there was no token passed
it would succeed. With my implementation above it will succeed as
expected.

If that's the case then prevent the creation of tokens in the
init_user_ns and be done with it. If you fallback anyway then this is
the correct solution.

Make this change, please. I'm not willing to support this weird fallback
stuff which is even hard to reason about.

Alright, added an extra check. Ok, so in summary I have the changes
below compared to v1 (plus a few extra LSM-related test cases added):

diff --git a/kernel/bpf/token.c b/kernel/bpf/token.c
index a86fccd57e2d..7d04378560fd 100644
--- a/kernel/bpf/token.c
+++ b/kernel/bpf/token.c

@@ -9,18 +9,22 @@
 #include <linux/user_namespace.h>
 #include <linux/security.h>

+static bool bpf_ns_capable(struct user_namespace *ns, int cap)
+{
+       return ns_capable(ns, cap) || (cap != CAP_SYS_ADMIN &&

ns_capable(ns, CAP_SYS_ADMIN));
+}
+
 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) ||
-                   (cap != CAP_SYS_ADMIN && ns_capable(token->userns,
CAP_SYS_ADMIN)))
-                       return security_bpf_token_capable(token, cap) == 0;
-       }
-       /* otherwise fallback to capable() checks */
-       return capable(cap) || (cap != CAP_SYS_ADMIN && capable(CAP_SYS_ADMIN));
+       struct user_namespace *userns;
+
+       /* BPF token allows ns_capable() level of capabilities */
+       userns = token ? token->userns : &init_user_ns;
+       if (!bpf_ns_capable(userns, cap))
+               return false;
+       if (token && security_bpf_token_capable(token, cap) < 0)
+               return false;
+       return true;
 }

 void bpf_token_inc(struct bpf_token *token)

@@ -32,7 +36,7 @@ static void bpf_token_free(struct bpf_token *token)
 {
        security_bpf_token_free(token);
        put_user_ns(token->userns);
-       kvfree(token);
+       kfree(token);
 }

 static void bpf_token_put_deferred(struct work_struct *work)

@@ -152,6 +156,12 @@ int bpf_token_create(union bpf_attr *attr)
                goto out_path;
        }

+       /* Creating BPF token in init_user_ns doesn't make much sense. */
+       if (current_user_ns() == &init_user_ns) {
+               err = -EOPNOTSUPP;
+               goto out_path;
+       }
+
        mnt_opts = path.dentry->d_sb->s_fs_info;
        if (mnt_opts->delegate_cmds == 0 &&
            mnt_opts->delegate_maps == 0 &&

@@ -179,7 +189,7 @@ int bpf_token_create(union bpf_attr *attr)
                goto out_path;
        }

-       token = kvzalloc(sizeof(*token), GFP_USER);
+       token = kzalloc(sizeof(*token), GFP_USER);
        if (!token) {
                err = -ENOMEM;
                goto out_file;