On Fri, Jan 12, 2024 at 11:17 AM Christian Brauner <brauner@xxxxxxxxxx> wrote:
>
> > > 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
> >
> > >
> > > (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;
