[PATCH bpf-next v2] bpf: Fix stack out-of-bounds write in cgroup link update

[PATCH bpf-next v2] bpf: Fix stack out-of-bounds write in cgroup link update

[Sanghyun Park]

The cgroup link update path checks only the program type. Several cgroup
hooks share a type while using different runtime contexts or verifier
contracts. A UDP6 sock_addr program can therefore replace a UDP4 program
and write beyond the four-byte ipc.addr context into adjacent fields of
the stack-local struct ipcm_cookie. The same omission lets an LSM_MAC
program replace an LSM_CGROUP program despite the incompatible return
semantics.

Validate replacement programs against the link attach type. Use the
existing per-type rules where applicable, and compare LSM
expected_attach_type explicitly because both flavors share
BPF_PROG_TYPE_LSM. Preserve legacy non-enforcing CGROUP_SKB
ingress/egress updates.

CGROUP_SKB programs do not require CAP_NET_ADMIN when loaded. That
permission is checked when the program is attached. Once the link exists,
updates are controlled through its FD, so BPF_LINK_UPDATE does not check
CAP_NET_ADMIN again. Keep this behavior and only validate the attach type
during link update.

Fixes: 0c991ebc8c69 ("bpf: Implement bpf_prog replacement for an active bpf_cgroup_link")
Signed-off-by: Sanghyun Park <sanghyun.park.cnu@gmail.com>
---
v2:
  - Extend validation from cgroup sock_addr programs to all cgroup program
    types, including exact LSM attach flavors.
  - Preserve legacy CGROUP_SKB ingress/egress replacement compatibility.
  - Keep the CGROUP_SKB CAP_NET_ADMIN check on attach, not link update.
v1: https://lore.kernel.org/r/20260805052858.2390918-3-sanghyun.park.cnu@gmail.com
---
 kernel/bpf/syscall.c | 28 ++++++++++++++++++++++++----
 1 file changed, 24 insertions(+), 4 deletions(-)

diff --git a/kernel/bpf/syscall.c b/kernel/bpf/syscall.c
index 8d111da886553c..de7a8b46814648 100644
--- a/kernel/bpf/syscall.c
+++ b/kernel/bpf/syscall.c
@@ -4462,7 +4462,8 @@ attach_type_to_prog_type(enum bpf_attach_type attach_type)
 }
 
 static int bpf_prog_attach_check_attach_type(const struct bpf_prog *prog,
-					     enum bpf_attach_type attach_type)
+					     enum bpf_attach_type attach_type,
+					     bool check_cap_net_admin)
 {
 	enum bpf_prog_type ptype;
 
@@ -4473,7 +4474,8 @@ static int bpf_prog_attach_check_attach_type(const struct bpf_prog *prog,
 	case BPF_PROG_TYPE_SK_LOOKUP:
 		return attach_type == prog->expected_attach_type ? 0 : -EINVAL;
 	case BPF_PROG_TYPE_CGROUP_SKB:
-		if (!bpf_token_capable(prog->aux->token, CAP_NET_ADMIN))
+		if (check_cap_net_admin &&
+		    !bpf_token_capable(prog->aux->token, CAP_NET_ADMIN))
 			/* cg-skb progs can be loaded by unpriv user.
 			 * check permissions at attach time.
 			 */
@@ -4596,7 +4598,7 @@ static int bpf_prog_attach(const union bpf_attr *attr)
 	if (IS_ERR(prog))
 		return PTR_ERR(prog);
 
-	if (bpf_prog_attach_check_attach_type(prog, attr->attach_type)) {
+	if (bpf_prog_attach_check_attach_type(prog, attr->attach_type, true)) {
 		bpf_prog_put(prog);
 		return -EINVAL;
 	}
@@ -5797,7 +5799,8 @@ static int link_create(union bpf_attr *attr, bpfptr_t uattr)
 		return PTR_ERR(prog);
 
 	ret = bpf_prog_attach_check_attach_type(prog,
-						attr->link_create.attach_type);
+						attr->link_create.attach_type,
+						true);
 	if (ret)
 		goto out;
 
@@ -5922,6 +5925,7 @@ static int link_update(union bpf_attr *attr)
 {
 	struct bpf_prog *old_prog = NULL, *new_prog;
 	struct bpf_link *link;
+	enum bpf_attach_type atype;
 	u32 flags;
 	int ret;
 
@@ -5959,6 +5963,22 @@ static int link_update(union bpf_attr *attr)
 		goto out_put_progs;
 	}
 
+	if (link->type == BPF_LINK_TYPE_CGROUP) {
+		atype = link->attach_type;
+		/*
+		 * BPF_LSM_MAC and BPF_LSM_CGROUP share BPF_PROG_TYPE_LSM, so
+		 * the helper's default prog-type check cannot distinguish them.
+		 */
+		if (new_prog->type == BPF_PROG_TYPE_LSM &&
+		    new_prog->expected_attach_type != atype)
+			ret = -EINVAL;
+		else
+			ret = bpf_prog_attach_check_attach_type(new_prog, atype,
+								false);
+		if (ret)
+			goto out_put_progs;
+	}
+
 	if (link->ops->update_prog)
 		ret = link->ops->update_prog(link, new_prog, old_prog);
 	else

Re: [PATCH bpf-next v2] bpf: Fix stack out-of-bounds write in cgroup link update

[Leon Hwang]

On 18/8/26 14:10, Sanghyun Park wrote:
> The cgroup link update path checks only the program type. Several cgroup
> hooks share a type while using different runtime contexts or verifier
> contracts. A UDP6 sock_addr program can therefore replace a UDP4 program
> and write beyond the four-byte ipc.addr context into adjacent fields of
> the stack-local struct ipcm_cookie. The same omission lets an LSM_MAC
> program replace an LSM_CGROUP program despite the incompatible return
> semantics.
> 
> Validate replacement programs against the link attach type. Use the
> existing per-type rules where applicable, and compare LSM
> expected_attach_type explicitly because both flavors share
> BPF_PROG_TYPE_LSM. Preserve legacy non-enforcing CGROUP_SKB
> ingress/egress updates.
> 
> CGROUP_SKB programs do not require CAP_NET_ADMIN when loaded. That
> permission is checked when the program is attached. Once the link exists,
> updates are controlled through its FD, so BPF_LINK_UPDATE does not check
> CAP_NET_ADMIN again. Keep this behavior and only validate the attach type
> during link update.
> 
> Fixes: 0c991ebc8c69 ("bpf: Implement bpf_prog replacement for an active bpf_cgroup_link")
> Signed-off-by: Sanghyun Park <sanghyun.park.cnu@gmail.com>
> ---
> v2:
>   - Extend validation from cgroup sock_addr programs to all cgroup program
>     types, including exact LSM attach flavors.
>   - Preserve legacy CGROUP_SKB ingress/egress replacement compatibility.
>   - Keep the CGROUP_SKB CAP_NET_ADMIN check on attach, not link update.
> v1: https://lore.kernel.org/r/20260805052858.2390918-3-sanghyun.park.cnu@gmail.com
> ---
>  kernel/bpf/syscall.c | 28 ++++++++++++++++++++++++----
>  1 file changed, 24 insertions(+), 4 deletions(-)
> 
> diff --git a/kernel/bpf/syscall.c b/kernel/bpf/syscall.c
> index 8d111da886553c..de7a8b46814648 100644
> --- a/kernel/bpf/syscall.c
> +++ b/kernel/bpf/syscall.c
> @@ -4462,7 +4462,8 @@ attach_type_to_prog_type(enum bpf_attach_type attach_type)
>  }
>  
>  static int bpf_prog_attach_check_attach_type(const struct bpf_prog *prog,
> -					     enum bpf_attach_type attach_type)
> +					     enum bpf_attach_type attach_type,
> +					     bool check_cap_net_admin)

Seems not a good choice to use 'bool check_cap_net_admin' here.

Better to factor out a helper to check CAP_NET_ADMIN first? Like:

static int bpf_prog_attach_check(const struct bpf_prog *prog,
				 enum bpf_attach_type attach_type)
{
	/* ... */
	if (prog->type == BPF_PROG_TYPE_CGROUP_SKB &&
	    !bpf_token_capable(prog->aux->token, CAP_NET_ADMIN))
		return -EPERM;

	return bpf_prog_attach_check_attach_type(prog, attach_type);
}

Thanks,
Leon

>  [...]

Re: [PATCH bpf-next v2] bpf: Fix stack out-of-bounds write in cgroup link update

[Sanghyun Park]

Hello Leon,

Thanks for the review.

> Seems not a good choice to use 'bool check_cap_net_admin' here.
>
> Better to factor out a helper to check CAP_NET_ADMIN first?

I tried to minimize the code changes and avoid introducing another helper, but
it seems that your approach is cleaner than this version. 

Let me try that way in v3, thanks!