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

[sashiko-bot@kernel.org]

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?

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