Re: [PATCH bpf-next v1] bpf: Fix func_info_aux desync after dead code elimination

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

Pre-existing issues:
- [High] Failure to update `env->exception_callback_subprog` during dead code elimination leads to an out-of-bounds array read and potential crash during JIT.
--

--- Patch [1]: [PATCH bpf-next v1] bpf: Fix func_info_aux desync after dead code elimination ---
commit f1d27d196a7979de10f6e09614e6262cf8069cae
Author: Kumar Kartikeya Dwivedi <[email protected]>

bpf: Fix func_info_aux desync after dead code elimination

The commit shifts func_info_aux together with func_info so the three parallel
arrays stay aligned after subprogram removal during dead code elimination.

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

> diff --git a/kernel/bpf/fixups.c b/kernel/bpf/fixups.c
> index c4bd70befbb5c..37ea85e974f14 100644
> --- a/kernel/bpf/fixups.c
> +++ b/kernel/bpf/fixups.c
> @@ -401,13 +401,17 @@ static int adjust_subprog_starts_after_remove(struct bpf_verifier_env *env,
>  			sizeof(*env->subprog_info) * move);
>  		env->subprog_cnt -= j - i;

[Severity: High]
This isn't a bug introduced by this patch, but should we also be adjusting
env->exception_callback_subprog here?

When dead code elimination removes an unused static function that comes before
an exception callback, env->subprog_cnt is decremented and subprograms are
shifted down. However, env->exception_callback_subprog is left unadjusted.

Later, in jit_subprogs(), the func array is allocated with the new, smaller
env->subprog_cnt. Accessing it using the outdated
env->exception_callback_subprog index could result in an out-of-bounds array
read and a potential kernel panic:

kernel/bpf/fixups.c:jit_subprogs() {
    ...
    prog->aux->bpf_exception_cb = (void *)func[env->exception_callback_subprog]->bpf_func;
    ...
}

Does env->exception_callback_subprog need to be updated to reflect the shift
when subprograms are removed?

>  
> -		/* remove func_info */
> +		/* remove func_info and its aux */
>  		if (aux->func_info) {
>  			move = aux->func_info_cnt - j;
>  
>  			memmove(aux->func_info + i,
>  				aux->func_info + j,
>  				sizeof(*aux->func_info) * move);
> +			if (aux->func_info_aux)
> +				memmove(aux->func_info_aux + i,
> +					aux->func_info_aux + j,
> +					sizeof(*aux->func_info_aux) * move);
>  			aux->func_info_cnt -= j - i;
>  			/* func_info->insn_off is set after all code rewrites,
>  			 * in adjust_btf_func() - no need to adjust

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