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

Thank you for your contribution! Sashiko AI review found 1 potential issue(s) to consider:

Pre-existing issues:
- [Critical] The patch selectively limits the `expected_attach_type` mismatch check to `BPF_PROG_TYPE_CGROUP_SOCK_ADDR`. This leaves `BPF_PROG_TYPE_CGROUP_SOCKOPT` unrestricted, allowing an attach type mismatch that leads to a verifier bounds tracking bypass and a NULL pointer dereference, which can be exploited for Local Privilege Escalation (LPE) or to cause a kernel panic.
--

--- Patch [1]: [PATCH bpf-next] bpf: Fix stack out-of-bounds write in cgroup link update ---
commit 842ffd1efbee299eada62f99bf59a538fc141873
Author: Sanghyun Park <[email protected]>

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

This commit limits the expected_attach_type check during cgroup link
updates to BPF_PROG_TYPE_CGROUP_SOCK_ADDR programs. This aims to fix
an out-of-bounds write while leaving update policies for other cgroup
program types unchanged.

> diff --git a/kernel/bpf/cgroup.c b/kernel/bpf/cgroup.c
> index d2da5063d8f81..65bf25deac4f1 100644
> --- a/kernel/bpf/cgroup.c
> +++ b/kernel/bpf/cgroup.c
> @@ -1055,6 +1055,10 @@ static int __cgroup_bpf_replace(struct cgroup *cgrp,
>  	if (link->link.prog->type != new_prog->type)
>  		return -EINVAL;
>  
> +	if (new_prog->type == BPF_PROG_TYPE_CGROUP_SOCK_ADDR &&
> +	    link->link.attach_type != new_prog->expected_attach_type)
> +		return -EINVAL;
> +

[Severity: Critical]
This is a pre-existing issue, but by explicitly restricting this check to
BPF_PROG_TYPE_CGROUP_SOCK_ADDR, does it allow BPF_PROG_TYPE_CGROUP_SOCKOPT
replacements to completely bypass attach_type validation?

If a user loads a BPF_PROG_TYPE_CGROUP_SOCKOPT program with an expected
type of BPF_CGROUP_GETSOCKOPT and another with BPF_CGROUP_SETSOCKOPT, could
they use bpf_link_update to replace a GETSOCKOPT link with the SETSOCKOPT
program?

The verifier expects bpf_get_retval() to return [-MAX_ERRNO, 0] for
SETSOCKOPT programs. If attached to a GETSOCKOPT link, could another program
on the hook set a positive retval, allowing the mismatched program to read
this value and bypass verifier bounds tracking?

Additionally, if a GETSOCKOPT program runs on a SETSOCKOPT hook, it maps
the return value to a dereference of ctx->current_task:

kernel/bpf/cgroup.c:cg_sockopt_convert_ctx_access() {
    ...
    *insn++ = BPF_LDX_MEM(BPF_FIELD_SIZEOF(struct bpf_sockopt_kern, current_task),
                          si->dst_reg, si->src_reg,
                          offsetof(struct bpf_sockopt_kern, current_task));
    *insn++ = BPF_LDX_MEM(BPF_FIELD_SIZEOF(struct task_struct, bpf_ctx),
                          si->dst_reg, si->dst_reg,
                          offsetof(struct task_struct, bpf_ctx));
    ...
}

Since current_task is not initialized in 
__cgroup_bpf_run_filter_setsockopt(), would this result in a NULL pointer
dereference when accessing current_task->bpf_ctx?

>  	hlist_for_each_entry(pl, progs, node) {
>  		if (pl->link == link) {
>  			found = true;

-- 
Sashiko AI review · https://sashiko.dev/#/patchset/[email protected]?part=1