Re: [PATCH bpf-next v9 04/23] bpf: Keep a call site's landing pad in insn_aux_data, add lookups

[sashiko-bot@kernel.org]

Thank you for your contribution! Sashiko AI review found 1 potential issue(s) to consider:
- [Medium] bpf: adjust_insn_aux_data() fails to shift cleanup_pad for BPF_PATCH_MOVE_TARGET
--

commit 1bdc64eca7dc9ba53a20b6b6c4d0e4448bdd4aa8
Author: Yonghong Song <yonghong.song@linux.dev>

bpf: Keep a call site's landing pad in insn_aux_data, add lookups

A call site's landing pad, if any, is kept in insn_aux_data as
cleanup_pad, so the three places that move instructions --
bpf_patch_insn_data(), verifier_remove_insns() and bpf_opt_remove_nops()
-- keep it in step. Add lookups for it and for calls to bpf_unwind() and
bpf_unwind_resume(); their users come in later patches.

> diff --git a/kernel/bpf/fixups.c b/kernel/bpf/fixups.c
> index 206cc9a614a95..64baee1a37e60 100644
> --- a/kernel/bpf/fixups.c
> +++ b/kernel/bpf/fixups.c
> @@ -244,11 +244,18 @@ static void adjust_insn_aux_data(struct bpf_verifier_env *env,
>  			data[i].non_stack_access =
>  				data[off + cnt - 1].non_stack_access;
>  			data[off + cnt - 1].non_stack_access = false;
> +			data[i].cleanup_pad = data[off + cnt - 1].cleanup_pad;
> +			data[off + cnt - 1].cleanup_pad = 0;
>  		} else if (bpf_is_mem_insn(insn + i)) {
>  			data[i].non_stack_access = true;
>  		}
>  	}
>  
> +	if (env->cleanup_info_cnt)
> +		for (i = 0; i < prog_len; i++)
> +			if (data[i].cleanup_pad > off + 1)
> +				data[i].cleanup_pad += cnt - 1;

[Severity: Medium]
Does this logic fail to adjust the landing pad when mode is
BPF_PATCH_MOVE_TARGET?

By unconditionally using off + 1, this appears to assume
BPF_PATCH_KEEP_TARGET semantics.

If bpf_patch_insn_data() is called with BPF_PATCH_MOVE_TARGET and the landing
pad is located exactly at the patched instruction, data[i].cleanup_pad would
be equal to off + 1 (the 1-based index). The condition
data[i].cleanup_pad > off + 1 evaluates to false, meaning the landing pad
index is not shifted.

As a result, the landing pad remains pointing at the newly inserted
instructions instead of following the original instruction shifted to
off + cnt - 1.

Could this cause the unwinder to jump to the wrong instruction offset,
potentially leading to state corruption during an exception unwind?

-- 
Sashiko AI review · https://sashiko.dev/#/patchset/20261008074959.2993751-1-yonghong.song@linux.dev?part=4