Re: [PATCH bpf v2] bpf: Enforce cgroup storage map consistency for freplace attach

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

New issues:
- [High] The function `bpf_freplace_check_tgt_prog` leaks `tgt_prog->aux->is_extended = true` if the cgroup storage map consistency check fails.

Pre-existing issues:
- [High] Failure in `bpf_arch_text_poke` during freplace attachment corrupts trampoline state and leaves a dangling pointer, permanently blocking future attachments.
--

--- Patch [1]: [PATCH bpf v2] bpf: Enforce cgroup storage map consistency for freplace attach ---
commit d906b268f92284796bc16bccd01e3ee3f9f415d5
Author: Aohan Mei <[email protected]>

bpf: Enforce cgroup storage map consistency for freplace attach

This commit adds a check in bpf_freplace_check_tgt_prog() to ensure that an
extension program references the exact same cgroup storage map as the target
program. If the maps differ, it returns -EINVAL to prevent verifier and
runtime inconsistencies.

> diff --git a/kernel/bpf/trampoline.c b/kernel/bpf/trampoline.c
> index 1a721fc4bef56..043bee6aaab2a 100644
> --- a/kernel/bpf/trampoline.c
> +++ b/kernel/bpf/trampoline.c

[ ... ]

> @@ -821,6 +823,23 @@ static int bpf_freplace_check_tgt_prog(struct bpf_prog *tgt_prog)
>  		return -EBUSY;
>  
>  	aux->is_extended = true;
> +
> +	/*
> +	 * At runtime the extension program inherits the target program's
> +	 * cgroup storage context (via prog_item->cgroup_storage), so its
> +	 * own map never provides storage; the verifier, however, bounds
> +	 * its accesses (size, flags, layout) by that map. Require both
> +	 * programs to reference the same map, matching the cookie
> +	 * semantics of the prog-array path.
> +	 */
> +	for_each_cgroup_storage_type(i) {
> +		struct bpf_map *tgt_map = tgt_prog->aux->cgroup_storage[i];
> +		struct bpf_map *prog_map = prog->aux->cgroup_storage[i];
> +
> +		if (prog_map && prog_map != tgt_map)
> +			return -EINVAL;

[Severity: High]
Does this code leak the aux->is_extended state?

If the cgroup storage map consistency check fails here and returns -EINVAL,
aux->is_extended remains permanently set to true. Will this cause future
attempts to add the target program to a prog_array map to fail with -EBUSY?

> +	}
> +
>  	return 0;
>  }

[ ... ]

> @@ -926,7 +945,7 @@ static int __bpf_trampoline_link_prog(struct bpf_tramp_node *node,
>  		/* Cannot attach extension if fentry/fexit are in use. */
>  		if (cnt)
>  			return -EBUSY;
> -		err = bpf_freplace_check_tgt_prog(tgt_prog);
> +		err = bpf_freplace_check_tgt_prog(tgt_prog, node->link->prog);
>  		if (err)
>  			return err;
>  		tr->extension_prog = node->link->prog;
> 		return bpf_arch_text_poke(tr->func.addr, BPF_MOD_NOP,
> 					  BPF_MOD_JUMP, NULL,
> 					  node->link->prog->bpf_func);

[Severity: High]
This is a pre-existing issue, but if bpf_arch_text_poke() fails here, does
it leave tr->extension_prog pointing to a failed program?

If the text modification fails and returns an error, the failed link will
subsequently be cleaned up and freed, which might leave tr->extension_prog
as a dangling pointer. Furthermore, tgt_prog->aux->is_extended is left as
true. Would future attachments to this trampoline see the non-NULL dangling
pointer and fail with -EBUSY?

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