Re: [PATCH bpf v6] bpf, cgroup: Fix storage null-ptr-deref after replacing prog

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

Pre-existing issues:
- [High] Missing expected_attach_type validation in __cgroup_bpf_replace allows attaching incompatible programs to cgroup hooks via BPF_LINK_UPDATE.
- [High] Lockless reads of link->prog race with concurrent link updates, leading to a Use-After-Free of the BPF program.
--

--- Patch [1]: [PATCH bpf v6] bpf, cgroup: Fix storage null-ptr-deref after replacing prog ---
commit 8ecaeff0e405145eaf841d2d2d05043d5e1a974d
Author: Pu Lehui <[email protected]>

bpf, cgroup: Fix storage null-ptr-deref after replacing prog

This commit fixes a null pointer dereference issue during bpf_prog
replacement for an active cgroup link. It prevents the panic by rejecting
link updates if the new program's cgroup storage is incompatible with the
existing program.

Link: https://lore.kernel.org/bpf/[email protected]
Link: https://lore.kernel.org/bpf/[email protected]

> diff --git a/kernel/bpf/cgroup.c b/kernel/bpf/cgroup.c
> index 4355ccb78a9c6..5cc9b2d92fd58 100644
> --- a/kernel/bpf/cgroup.c
> +++ b/kernel/bpf/cgroup.c

[ ... ]

> @@ -1056,6 +1070,9 @@ static int __cgroup_bpf_replace(struct cgroup *cgrp,
>  	if (!found)
>  		return -ENOENT;
>  
> +	if (!cgroup_bpf_storages_compatible(link->link.prog, new_prog))
> +		return -EINVAL;
> +

[Severity: High]
This is a pre-existing issue, but does this function also need to validate
expected_attach_type before allowing the replacement?

In __cgroup_bpf_replace(), the code currently checks:

    if (link->link.prog->type != new_prog->type)
        return -EINVAL;

However, it does not check expected_attach_type. Could an attacker with
CAP_BPF use BPF_LINK_UPDATE to swap a program with a different
expected_attach_type (for example, swapping BPF_CGROUP_UDP4_SENDMSG onto a
BPF_CGROUP_INET4_CONNECT link)?

If the verifier validations are bypassed at runtime, the incorrect context
structures could lead to out-of-bounds accesses and guaranteed NULL pointer
dereferences when the program executes on the hook.

>  	cgrp->bpf.revisions[atype] += 1;
>  	old_prog = xchg(&link->link.prog, new_prog);

[Severity: High]
This isn't a bug introduced by this patch, but can this xchg() lead to a
use-after-free of the BPF program when racing with lockless readers?

Following this swap, bpf_prog_put() releases the old program, which is freed
after an RCU grace period. However, generic reader functions access
link->prog locklessly and in preemptible context.

For example, in kernel/bpf/syscall.c:bpf_link_get_info_by_fd():

    if (link->prog)
        info.prog_id = link->prog->aux->id;

And in kernel/bpf/syscall.c:bpf_link_show_fdinfo():

    const struct bpf_prog *prog = link->prog;
    ...
    if (prog) {
        bin2hex(prog_tag, prog->tag, sizeof(prog->tag));

Since these readers can sleep on copy_to_user() or seq_printf() and do not
hold rcu_read_lock() or take a reference count, could preemption cause the
RCU grace period to expire before prog->aux or prog->tag is dereferenced?

>  	replace_effective_prog(cgrp, atype, pl);

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