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

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 <[email protected]>
> ---
> v3:
>   - Factor the CGROUP_SKB CAP_NET_ADMIN check into an attach-only helper.
> v2: https://lore.kernel.org/r/[email protected]
>   - 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/[email protected]
> ---
>  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;
> [...]