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@xxxxxxxxxxxxxxxxxxx> 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@xxxxxxxxxx> 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.
