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

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 <[email protected]>
> ---
> 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/[email protected]
> ---
>  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

>  [...]