Re: [PATCH] bpf: netns: require CAP_NET_ADMIN for link targets

[bot+bpf-ci@kernel.org]

[-- Attachment #1: Type: text/plain, Size: 3651 bytes --]

> bpf: netns: require CAP_NET_ADMIN for link targets

TL;DR:
- Flow dissector BPF_PROG_ATTACH may still bypass the new netns capability
  check, unverified
- Check may wrongly reject CAP_SYS_ADMIN-only and sk_lookup users; the commit
  message doesn't say so

> diff --git a/kernel/bpf/net_namespace.c b/kernel/bpf/net_namespace.c
> index 81006a242618..0ca29c20ba85 100644
> --- a/kernel/bpf/net_namespace.c
> +++ b/kernel/bpf/net_namespace.c

[ ... ]

> @@ -488,6 +489,10 @@ int netns_bpf_link_create(const union bpf_attr *attr, struct bpf_prog *prog)
>  	net = get_net_ns_by_fd(attr->link_create.target_fd);
>  	if (IS_ERR(net))
>  		return PTR_ERR(net);
> +	if (!ns_capable(net->user_ns, CAP_NET_ADMIN)) {
> +		err = -EPERM;
> +		goto out_put_net;
> +	}
>
>  	net_link = kzalloc_obj(*net_link, GFP_USER);
>  	if (!net_link) {

Does this close the token-versus-netns hole for BPF_PROG_TYPE_FLOW_DISSECTOR?

Flow dissector programs can be attached through both BPF_LINK_CREATE and
BPF_PROG_ATTACH, and the BPF_PROG_ATTACH path does not go through this
function:

bpf_prog_attach()
  netns_bpf_prog_attach()
    net = current->nsproxy->net_ns;
    ...

bpf_prog_attach_check_attach_type() has no FLOW_DISSECTOR case, and
netns_bpf_prog_attach() and netns_bpf_prog_detach() have no ns_capable()
check on net->user_ns.

A task that is root in a userns, and so passes bpf_token_capable(token,
CAP_NET_ADMIN) at BPF_PROG_LOAD time through a delegated token, but is still
a member of a netns owned by a different userns (for example init_net after
unshare -U), could load a flow dissector program with the token and call
BPF_PROG_ATTACH(BPF_FLOW_DISSECTOR).  That appears to succeed without
CAP_NET_ADMIN over that netns, which is the outcome the commit message says
this patch prevents:

> a token can be used to attach a BPF program to a netns it does not
> control.

This is pre-existing code rather than something this patch breaks, but given
the Fixes: and Cc: stable framing, should the userns check live in a shared
helper that netns_bpf_prog_attach() and netns_bpf_prog_detach() also use, or
should the changelog state that limitation?  (SK_LOOKUP is link-only and is
covered by this check.)

Also, should this check mirror bpf_ns_capable()?

The new check is a bare ns_capable(net->user_ns, CAP_NET_ADMIN), but the
token code treats CAP_SYS_ADMIN as satisfying a CAP_NET_ADMIN requirement:

kernel/bpf/token.c:bpf_ns_capable() {
	return ns_capable(ns, cap) || (cap != CAP_SYS_ADMIN && ns_capable(ns, CAP_SYS_ADMIN));
}

and other attach-time checks use that convention, for example in
bpf_prog_attach_check_attach_type():

	if (!bpf_token_capable(prog->aux->token, CAP_NET_ADMIN))
		return -EPERM;

With the patch, a caller that has CAP_SYS_ADMIN but not CAP_NET_ADMIN can
still BPF_PROG_LOAD a flow dissector program, but BPF_LINK_CREATE now fails
with -EPERM, while BPF_PROG_ATTACH of a flow dissector still works for the
same caller.  The same applies to a CAP_BPF-only caller loading an sk_lookup
program, since BPF_PROG_TYPE_SK_LOOKUP is not in is_net_admin_prog_type().

The commit message does not mention this user-visible narrowing, and the
patch is tagged Cc: stable.  If the change for CAP_BPF-only sk_lookup users
is intended, could the commit message say so?  For the CAP_SYS_ADMIN case,
would falling back to CAP_SYS_ADMIN in net->user_ns keep the permission
model consistent with load time?


---
AI reviewed your patch. Please fix the bug or email reply why it's not a bug.
See: https://github.com/kernel-patches/vmtest/blob/master/ci/claude/README.md

CI run summary: https://github.com/kernel-patches/bpf/actions/runs/41521741043