Re: [PATCH bpf-next v7 02/11] bpf: Disallow interpreter fallback for user BPF_ADDR_SPACE_CAST insn

[sashiko-bot@kernel.org]

Thank you for your contribution! Sashiko AI review found 2 potential issue(s) to consider:

New issues:
- [Medium] Missing verifier log message for non-JITed programs rejected due to `has_addr_space_cast_insn`.

Pre-existing issues:
- [Critical] Incomplete fix allows BPF interpreter fallback for arena programs, leading to a kernel panic.
--

commit 37ba328d18b1ffa46e00f11cf84efa654fd229ec
Author: Leon Hwang <leon.hwang@linux.dev>

bpf: Disallow interpreter fallback for user BPF_ADDR_SPACE_CAST insn

Since interpreter is unable to handle the 'insn_is_cast_user()' insn,
require JIT in __bpf_prog_select_runtime() when the prog has the insn.

> diff --git a/kernel/bpf/core.c b/kernel/bpf/core.c
> index 649cce41e13f3..49398b5bd172d 100644
> --- a/kernel/bpf/core.c
> +++ b/kernel/bpf/core.c
> @@ -2620,6 +2620,7 @@ struct bpf_prog *__bpf_prog_select_runtime(struct bpf_verifier_env *env, struct
>  		goto finalize;
>  
>  	if (IS_ENABLED(CONFIG_BPF_JIT_ALWAYS_ON) ||
> +	    fp->aux->has_addr_space_cast_insn ||
>  	    bpf_prog_has_kfunc_call(fp))
>  		jit_needed = true;

[Severity: Medium]
If JIT is disabled or fails, returning -ENOTSUPP here will reject the
program load silently without a verifier log message. Unlike
bpf_prog_has_kfunc_call() which provides a helpful verbose() log in
bpf_fixup_call_args():

	if (has_kfunc_call) {
		verbose(env, "calling kernel functions are not allowed in non-JITed programs\n");
		return -EINVAL;
	}

Users encountering this for has_addr_space_cast_insn will not see an
explanation for the EOPNOTSUPP error. Could we add a verifier log message
explaining why JIT is required here?

> diff --git a/kernel/bpf/fixups.c b/kernel/bpf/fixups.c
> index 3cf2cc6e3ab66..2d5958774b61b 100644
> --- a/kernel/bpf/fixups.c
> +++ b/kernel/bpf/fixups.c
> @@ -1523,6 +1523,8 @@ int bpf_do_misc_fixups(struct bpf_verifier_env *env)
>  				insn->off = 0;
>  				insn->imm = 0;
>  			} /* cast from as(0) to as(1) should be handled by JIT */
> +			if (insn_is_cast_user(insn))
> +				prog->aux->has_addr_space_cast_insn = true;

[Severity: Critical]
This is a pre-existing issue, but the patch attempts to fix interpreter
fallback for BPF_ADDR_SPACE_CAST and fails to address the underlying panic
with arena programs.

If a program uses an arena and obtains a PTR_TO_ARENA without a cast_user
instruction (for instance, using cast_kern), or if the arena map has
BPF_F_NO_USER_CONV set, has_addr_space_cast_insn will not be set.

In the case of BPF_F_NO_USER_CONV, the instruction is rewritten to
BPF_ALU | BPF_MOV | BPF_X right before the insn_is_cast_user() check.

Memory accesses using PTR_TO_ARENA are rewritten by bpf_do_misc_fixups()
to use BPF_PROBE_MEM32 or BPF_PROBE_MEM32SX. If these arena programs fall
back to the interpreter, wouldn't they hit the BUG_ON(1) in ___bpf_prog_run()
because it lacks handlers for these opcodes?

kernel/bpf/core.c:___bpf_prog_run() {
    ...
	default_label:
		pr_warn("BPF interpreter: unknown opcode %02x (imm: 0x%x)\n",
			insn->code, insn->imm);
		BUG_ON(1);
    ...
}

Should jit_needed be enforced in __bpf_prog_select_runtime() for all
programs using fp->aux->arena instead?

-- 
Sashiko AI review · https://sashiko.dev/#/patchset/20260622143557.22955-1-leon.hwang@linux.dev?part=2