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

[Yonghong Song <yonghong.song@linux.dev>]

On 10/9/26 6:28 AM, sashiko-bot@kernel.org wrote:
> 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

Looks like there are no new change with BPF_PATCH_MOVE_TARGET, so I think
we do not need to do the above. The below is the comment in v9:

====

Okay, commit "bpf: Retarget indirect jump targets across prologue prepends"
introduced BPF_PATCH_MOVE_TARGET and merged last Friday. It solved three
cases for ops->gen_epilogue, ops->gen_prologue || env->seen_direct_write, and
stack slots for subprogs. ops->gen_epilogue has been rejected in patch 5.
We cannot allow ops->gen_epilogue since it may silently exit.
For other cases in "bpf: Retarget indirect jump targets across prologue prepends",
The above commit should already handle this.

For the other two, cleanup_pad == off + 1 cannot happen, because MOVE only
patches an entry insn and a landing pad cannot start at insn 0: the entry is
always walked outside a pad first, so reaching it again from an unwind
fails bpf_exc_check_insn() with "insn %u runs both inside and outside a
landing pad". Pads after off are shifted by the existing `> off + 1`, and a
covered call at off moves with its insn_aux_data.

====