Re: [PATCH bpf-next v10 04/24] 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:
- [High] bpf: adjust_insn_aux_data fails to shift cleanup_pad for BPF_PATCH_MOVE_TARGET
--

commit 8654da066c194611f7e2de5fef00da7ac62a0070
Author: Yonghong Song <yonghong.song@linux.dev>

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

This commit tracks a call site's landing pad in insn_aux_data as cleanup_pad.
It updates adjust_insn_aux_data(), verifier_remove_insns(), and
bpf_opt_remove_nops() to keep this data in sync when instructions are patched
or removed.

> 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: High]
Does this check properly account for the BPF_PATCH_MOVE_TARGET patch mode?

When bpf_patch_insn_data() is called with BPF_PATCH_MOVE_TARGET (for example,
when inserting a prologue), the original instruction is shifted to the end of
the newly inserted sequence. If a landing pad points exactly to the patched
instruction:

    data[i].cleanup_pad == off + 1

This condition would evaluate to false, leaving the cleanup_pad unadjusted.

As a result, if a subprogram with a landing pad at its first instruction
receives a prologue, wouldn't the unwinder incorrectly jump to the prologue
instead of the original instruction?

Could this lead to stack or frame pointer corruption by re-executing prologue
stack setups during unwinding? Other locations handling jumps typically use a
ternary to handle this, such as checking if it is greater than:

    (mode == BPF_PATCH_MOVE_TARGET) ? off : off + 1

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