Re: [PATCH bpf-next v8 04/22] bpf: Add lookups for exception cleanup resumes and landing pads

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

On 10/1/26 6:48 AM, sashiko-bot@kernel.org wrote:
> Thank you for your contribution! Sashiko AI review found 1 potential issue(s) to consider:
> - [High] bpf: failure to adjust cleanup_pad for BPF_PATCH_MOVE_TARGET allows arbitrary read/crash
> --
>
> commit 1a4d7ce9c87bb54c2feec4648d42fa1046ccb8b1
> Author: Yonghong Song <yonghong.song@linux.dev>
>
> bpf: Add lookups for exception cleanup resumes and landing pads
>
> This commit adds lookups to exception.c to recognize calls to bpf_unwind()
> and bpf_unwind_resume(). It also updates bpf_patch_insn_data(),
> verifier_remove_insns(), and bpf_opt_remove_nops() to keep the landing pads
> of call sites properly synchronized.
>
>> diff --git a/kernel/bpf/fixups.c b/kernel/bpf/fixups.c
>> index 37cf130ebb57b..5b7fe4ba610b3 100644
>> --- a/kernel/bpf/fixups.c
>> +++ b/kernel/bpf/fixups.c
> [ ... ]
>> @@ -268,11 +268,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]
> Could this fail to adjust the cleanup_pad correctly when the mode is
> BPF_PATCH_MOVE_TARGET in adjust_insn_aux_data()?
>
> The condition checks data[i].cleanup_pad > off + 1, which works well for
> BPF_PATCH_KEEP_TARGET, but it appears to miss BPF_PATCH_MOVE_TARGET where
> the target should follow the original instruction to the end of the patched
> sequence.
>
> When a prologue is inserted at offset 0 (for example, during
> bpf_convert_ctx_accesses), exception landing pads pointing to offset 0 are
> not shifted. If a user sets an exception landing pad at offset 0, unwinding
> would cause execution to jump to the newly inserted prologue instead of the
> original instruction.
>
> Since the prologue for program types like TC expects R1 to contain the CTX
> pointer, and R1 is a caller-saved register containing whatever was placed
> there before calling bpf_unwind(), could dereferencing this
> attacker-controlled R1 during an exception unwind lead to an arbitrary memory
> read or a kernel panic?
>
> Should this check be dependent on the mode, such as:
> data[i].cleanup_pad > (mode == BPF_PATCH_MOVE_TARGET ? off : off + 1)
> to properly account for both modes?
>
This patch should be okay. The following is what I replied in v8 for this patch:

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.

So I think this patch should be okay.