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 <[email protected]>
One nit below, others lgtm:
Acked-by: Leon Hwang <[email protected]>
> ---
> 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/[email protected]
> - 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 | 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
