Re: [PATCH bpf-next v2 07/22] bpf: Let the patch list remove instructions

[sashiko-bot@kernel.org]

Thank you for your contribution! Sashiko AI review found 1 potential issue(s) to consider:
- [High] bpf: premature prog->len update breaks bpf_find_subprog() in bpf_patch_list_commit()
--

commit 36c5b3c1e86de2cef14800f7c3faaea5d0fafe3d
Author: Qiliang Yuan <odys.yuan@gmail.com>

bpf: Let the patch list remove instructions

This commit adds bpf_patch_list_del() to allow the verifier's patch list to
queue instruction removals alongside replacements, updating line info,
subprogs, function pointers, instruction arrays, and offloaded programs when
committed.

> diff --git a/kernel/bpf/fixups.c b/kernel/bpf/fixups.c
> index 940265cb80ebf..1a1a374a076b1 100644
> --- a/kernel/bpf/fixups.c
> +++ b/kernel/bpf/fixups.c
[ ... ]
> @@ -514,25 +624,43 @@ int bpf_patch_list_commit(struct bpf_verifier_env *env)
>  	env->insn_aux_data_len = img.new_cnt;
>  	data = NULL;
>  
> +	/* before subprogs are removed, since it looks at them */
>  	for (i = 0; i < env->func_ptr_cnt; i++) {
>  		struct bpf_func_ptr *fp = &env->func_ptrs[i];
> +		int subprog;
>  
>  		if (fp->xlated_off == BPF_FUNC_PTR_DELETED)
>  			continue;
> +		if (bpf_patch_image_deleted(&img, fp->xlated_off)) {
> +			subprog = bpf_find_subprog(env, fp->xlated_off);
> +			if (subprog <= 0 ||
> +			    img.new_off[env->subprog_info[subprog].start] ==
> +			    img.new_off[env->subprog_info[subprog + 1].start]) {
> +				fp->xlated_off = BPF_FUNC_PTR_DELETED;
> +				continue;
> +			}
> +		}

[Severity: High]
Can calling bpf_find_subprog() here with the old-image offset fp->xlated_off
falsely mark a live function pointer as BPF_FUNC_PTR_DELETED when the program
shrinks?

Earlier in bpf_patch_list_commit(), bpf_patch_image_install() updates
prog->len to img.new_cnt:

kernel/bpf/core.c:bpf_patch_image_install() {
	memcpy(prog->insnsi, img->insns, sizeof(*img->insns) * img->new_cnt);
	prog->len = img->new_cnt;
    ...
}

When bpf_patch_list_commit() then calls
bpf_find_subprog()->bpf_find_containing_subprog(), it checks the old-image
offset against the already-shrunk env->prog->len:

kernel/bpf/verifier.c:bpf_find_containing_subprog() {
	if (off >= env->prog->len || off < 0 || env->subprog_cnt == 0)
		return NULL;
    ...
}

If a live static subprogram near the end of the program has its entry
instruction removed (for example, a nop or simplified branch removed in
bpf_opt_remove_nops()) and earlier instruction removals shrink the program
such that fp->xlated_off >= img.new_cnt, bpf_find_subprog() will return
-ENOENT.

Wouldn't that cause the subprog <= 0 check to set fp->xlated_off to
BPF_FUNC_PTR_DELETED even though the subprogram still has live instructions,
causing resolve_func_ptrs() during bpf_jit_subprogs() to write NULL into the
read-only map slot used by callx?

-- 
Sashiko AI review · https://sashiko.dev/#/patchset/20261030-bpf-verifier-patch-batch-v2-0-55cd43439f7c@gmail.com?part=7