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

From: Christian Brauner <brauner@kernel.org>
To: Andrii Nakryiko <andrii.nakryiko@gmail.com>
Cc: Linus Torvalds <torvalds@linuxfoundation.org>,
	 Andrii Nakryiko <andrii@kernel.org>,
	bpf@vger.kernel.org, netdev@vger.kernel.org, paul@paul-moore.com,
	 linux-fsdevel@vger.kernel.org,
	linux-security-module@vger.kernel.org, kernel-team@meta.com
Subject: Re: [PATCH bpf-next 03/29] bpf: introduce BPF token object
Date: Mon, 8 Jan 2024 13:02:37 +0100	[thread overview]
Message-ID: <20240108-gasheizung-umstand-a36d89ed36b7@brauner> (raw)
In-Reply-To: <CAEf4Bzb6jnJL98SLPJB7Vjxo_O33W8HjJuAsyP3+6xigZtsTkA@mail.gmail.com>

On Fri, Jan 05, 2024 at 02:18:40PM -0800, Andrii Nakryiko wrote:
> On Fri, Jan 5, 2024 at 1:45 PM Linus Torvalds
> <torvalds@linuxfoundation.org> 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@kernel.org> 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

NAK on not restricting this to not erroring out on current_user_ns()
!= token->user_ns. I've said this multiple times before.