Re: [PATCH bpf-next v4 1/2] bpf: Validate program attach type during link update

[Leon Hwang <leon.hwang@linux.dev>]

On 2026/8/31 10:39, Sanghyun Park wrote:
> Cgroup link updates compare only the program type. Programs verified
> for different hooks can share a type, so a UDP6 sock_addr program can
> replace a UDP4 program and write beyond the four-byte ipc.addr field in
> the stack-local struct ipcm_cookie. The same gap permits incompatible
> LSM_MAC and LSM_CGROUP replacements.
> 
> Validate every replacement program against the link attach type before
> dispatching to the link-specific update operation. Keep the CGROUP_SKB
> CAP_NET_ADMIN check at initial attach so pinned or delegated link FDs can
> still be updated after a capability drop.
> 
> Fixes: 0c991ebc8c69 ("bpf: Implement bpf_prog replacement for an active bpf_cgroup_link")
> Signed-off-by: Sanghyun Park <sanghyun.park.cnu@gmail.com>

One nit below, others lgtm:

Acked-by: Leon Hwang <leon.hwang@linux.dev>

> ---
> v4:
>   - Validate every program link update through the shared attach-type helper.
>   - Fold the LSM attach-flavor check into that helper.
>   - Keep the CGROUP_SKB permission check in the attach-only wrapper and rename
>     the wrapper to describe that role.
>   - Follow the preferred BPF multi-line comment style.
>   - Add focused regression selftests as patch 2/2.
> v3: https://lore.kernel.org/r/20260821084726.3769957-2-sanghyun.park.cnu@gmail.com
>   - 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 | 31 ++++++++++++++++++++++---------
>  1 file changed, 22 insertions(+), 9 deletions(-)
> 
> diff --git a/kernel/bpf/syscall.c b/kernel/bpf/syscall.c
> index 6874ba1424af..84823149dffa 100644
> --- a/kernel/bpf/syscall.c
> +++ b/kernel/bpf/syscall.c
> @@ -4481,14 +4481,9 @@ static int bpf_prog_attach_check_attach_type(const struct bpf_prog *prog,
>  	case BPF_PROG_TYPE_CGROUP_SOCK_ADDR:
>  	case BPF_PROG_TYPE_CGROUP_SOCKOPT:
>  	case BPF_PROG_TYPE_SK_LOOKUP:
> +	case BPF_PROG_TYPE_LSM:
>  		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 +4537,20 @@ static int bpf_prog_attach_check_attach_type(const struct bpf_prog *prog,
>  	}
>  }
>  
> +static int bpf_prog_attach_check_perm(const struct bpf_prog *prog,
> +				      enum bpf_attach_type attach_type)
> +{
> +	/*
> +	 * CGROUP_SKB programs can be loaded by unprivileged users, so check
> +	 * permissions at attach time.
> +	 */
> +	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);
> +}
> +
>  static bool is_cgroup_prog_type(enum bpf_prog_type ptype, enum bpf_attach_type atype,
>  				bool check_atype)
>  {
> @@ -4606,7 +4615,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_perm(prog, attr->attach_type)) {
>  		bpf_prog_put(prog);
>  		return -EINVAL;
>  	}
> @@ -5805,8 +5814,8 @@ 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_perm(prog,
> +					 attr->link_create.attach_type);

NIT: fold into one line. 100 chars are allowed per line.

Thanks,
Leon

>  	if (ret)
>  		goto out;
>  
> @@ -5968,6 +5977,10 @@ static int link_update(union bpf_attr *attr)
>  		goto out_put_progs;
>  	}
>  
> +	ret = bpf_prog_attach_check_attach_type(new_prog, link->attach_type);
> +	if (ret)
> +		goto out_put_progs;
> +
>  	if (link->ops->update_prog)
>  		ret = link->ops->update_prog(link, new_prog, old_prog);
>  	else