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

[PATCH bpf-next v3] 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>
---
v3:
  - Factor the CGROUP_SKB CAP_NET_ADMIN check into an attach-only helper.
v2: https://lore.kernel.org/r/20260818061021.2551771-2-sanghyun.park.cnu@gmail.com
  - 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 | 41 ++++++++++++++++++++++++++++++++---------
 1 file changed, 32 insertions(+), 9 deletions(-)

diff --git a/kernel/bpf/syscall.c b/kernel/bpf/syscall.c
index 6874ba1424af05..dfea337ff25ea5 100644
--- a/kernel/bpf/syscall.c
+++ b/kernel/bpf/syscall.c
@@ -4483,12 +4483,6 @@ 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))
-			/* cg-skb progs can be loaded by unpriv user.
-			 * check permissions at attach time.
-			 */
-			return -EPERM;
-
 		ptype = attach_type_to_prog_type(attach_type);
 		if (prog->type != ptype)
 			return -EINVAL;
@@ -4542,6 +4536,19 @@ static int bpf_prog_attach_check_attach_type(const struct bpf_prog *prog,
 	}
 }
 
+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))
+		/* cg-skb progs can be loaded by unpriv user.
+		 * check permissions at attach time.
+		 */
+		return -EPERM;
+
+	return bpf_prog_attach_check_attach_type(prog, attach_type);
+}
+
 static bool is_cgroup_prog_type(enum bpf_prog_type ptype, enum bpf_attach_type atype,
 				bool check_atype)
 {
@@ -4606,7 +4613,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(prog, attr->attach_type)) {
 		bpf_prog_put(prog);
 		return -EINVAL;
 	}
@@ -5805,8 +5812,7 @@ static int link_create(union bpf_attr *attr, bpfptr_t uattr)
 	if (IS_ERR(prog))
 		return PTR_ERR(prog);
 
-	ret = bpf_prog_attach_check_attach_type(prog,
-						attr->link_create.attach_type);
+	ret = bpf_prog_attach_check(prog, attr->link_create.attach_type);
 	if (ret)
 		goto out;
 
@@ -5931,6 +5937,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;
 
@@ -5968,6 +5975,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);
+		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 v3] bpf: Fix stack out-of-bounds write in cgroup link update

[sashiko-bot]

Thank you for your contribution! Sashiko AI review found 1 potential issue(s) to consider:
- [Low] A multi-line comment violates the mandatory BPF subsystem comment style.
--

--- Patch [1]: [PATCH bpf-next v3] bpf: Fix stack out-of-bounds write in cgroup link update ---
commit 3542a649528edfe00a99f7439585e0f6e14c0cc5
Author: Sanghyun Park <sanghyun.park.cnu@gmail.com>

bpf: Fix stack out-of-bounds write in cgroup link update

This commit validates replacement programs against the link attach type during
cgroup link updates to prevent out-of-bounds writes on the stack. It restricts
replacements between incompatible BPF program types that share the same main
type but have different context or return semantics. A new helper function is
introduced to factor out the capability checks for CGROUP_SKB programs.

> diff --git a/kernel/bpf/syscall.c b/kernel/bpf/syscall.c
> index 6874ba1424af0..dfea337ff25ea 100644
> --- a/kernel/bpf/syscall.c
> +++ b/kernel/bpf/syscall.c

[ ... ]

> @@ -4542,6 +4536,19 @@ static int bpf_prog_attach_check_attach_type(const struct bpf_prog *prog,
>  	}
>  }
>  
> +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))
> +		/* cg-skb progs can be loaded by unpriv user.
> +		 * check permissions at attach time.
> +		 */

[Severity: Low]
This isn't a bug, but does this violate the modern BPF multi-line comment
style? The BPF subsystem guidelines prefer having the opening /* on its
own line.

> +		return -EPERM;
> +
> +	return bpf_prog_attach_check_attach_type(prog, attach_type);
> +}
> +

-- 
Sashiko AI review · https://sashiko.dev/#/patchset/20260821084726.3769957-2-sanghyun.park.cnu@gmail.com?part=1

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

[bot+bpf-ci]

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

> diff --git a/kernel/bpf/syscall.c b/kernel/bpf/syscall.c
> index 6874ba1424af..dfea337ff25e 100644
> --- a/kernel/bpf/syscall.c
> +++ b/kernel/bpf/syscall.c

[ ... ]

> @@ -4542,6 +4536,19 @@ static int bpf_prog_attach_check_attach_type(const struct bpf_prog *prog,
>  	}
>  }
>
> +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))
> +		/* cg-skb progs can be loaded by unpriv user.
> +		 * check permissions at attach time.
> +		 */
> +		return -EPERM;
> +
> +	return bpf_prog_attach_check_attach_type(prog, attach_type);
> +}
> +

This isn't a bug, but would a name that mentions the permission gate
read more clearly next to bpf_prog_attach_check_attach_type()?

The new helper bpf_prog_attach_check() sits near
bpf_prog_attach_check_attach_type() in the same file, and the two names
differ only by a suffix while the security-relevant difference is which
one enforces CAP_NET_ADMIN.

link_update() deliberately calls the longer-named, non-enforcing one
(bpf_prog_attach_check_attach_type), and bpf_prog_attach() plus
link_create() call the shorter, enforcing one (bpf_prog_attach_check).
A name like bpf_prog_attach_check_perm would make the choice at each of
the three call sites self-evident.

[ ... ]


---
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/32466070697

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

[Leon Hwang]

On 21/8/26 16:47, 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.

Any issue of checking CAP_NET_ADMIN for BPF_LINK_UPDATE?

If no, checking CAP_NET_ADMIN for BPF_LINK_UPDATE looks okay.

> 
> Fixes: 0c991ebc8c69 ("bpf: Implement bpf_prog replacement for an active bpf_cgroup_link")
> Signed-off-by: Sanghyun Park <sanghyun.park.cnu@gmail.com>
> ---
> v3:
>   - Factor the CGROUP_SKB CAP_NET_ADMIN check into an attach-only helper.
> v2: https://lore.kernel.org/r/20260818061021.2551771-2-sanghyun.park.cnu@gmail.com
>   - 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 | 41 ++++++++++++++++++++++++++++++++---------
>  1 file changed, 32 insertions(+), 9 deletions(-)
> 
> diff --git a/kernel/bpf/syscall.c b/kernel/bpf/syscall.c
> index 6874ba1424af05..dfea337ff25ea5 100644
> --- a/kernel/bpf/syscall.c
> +++ b/kernel/bpf/syscall.c
> @@ -4483,12 +4483,6 @@ static int bpf_prog_attach_check_attach_type(const struct bpf_prog *prog,
>  	case BPF_PROG_TYPE_SK_LOOKUP:
+	case BPF_PROG_TYPE_LSM:

Add a 'case' here instead of checking it in link_update()?

And, a selftest is needed to verify the issue and the fix.

Thanks,
Leon

>  		return attach_type == prog->expected_attach_type ? 0 : -EINVAL;
> [...]