Re: [PATCH bpf-next 03/29] bpf: introduce BPF token object

[Christian Brauner <brauner@kernel.org>]

On Sat, Jan 13, 2024 at 06:29:33PM -0800, Andrii Nakryiko wrote:
> On Fri, Jan 12, 2024 at 11:17 AM Christian Brauner <brauner@kernel.org> 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;

Thank you! Looks good,

Acked-by: Christian Brauner <brauner@kernel.org>