Ok, I've gone through the whole series now, and I don't find anything
objectionable.
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));
}
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?
Linus
