Re: [PATCH bpf-next v3 45/45] selftests/bpf: Test callx of one of a set of functions

[sashiko-bot@kernel.org]

Thank you for your contribution! Sashiko AI review found 1 potential issue(s) to consider:
- [Medium] selftests/bpf: missing __load_if_JITed() in callx_one_of_two_not_written()
--

commit 64c4b960a70c853d9b8785754e157dac9645cef4
Author: Alexei Starovoitov <ast@kernel.org>

selftests/bpf: Test callx of one of a set of functions

This commit adds BPF verifier selftests covering indirect function calls
(callx) where a register holds a pointer to one of a set of functions loaded
from a read-only table, as well as declared argument liveness at callx.

> diff --git a/tools/testing/selftests/bpf/progs/verifier_stack_arg.c b/tools/testing/selftests/bpf/progs/verifier_stack_arg.c
> index e3f5e4d14b759..c642675dd92bc 100644
> --- a/tools/testing/selftests/bpf/progs/verifier_stack_arg.c
> +++ b/tools/testing/selftests/bpf/progs/verifier_stack_arg.c
> @@ -572,6 +572,60 @@ __naked void null_check_of_stack_arg(void)
>  	: __clobber_all);
>  }
>  
> +static __naked __noinline __used long adds_arg6_arg7(long a, long b, long c, long d, long e,
> +						     long f, long g)
> +{
> +	asm volatile (
> +	"r0 = *(u64 *)(r11 + 8);"
> +	"r1 = *(u64 *)(r11 + 16);"
> +	"r0 += r1;"
> +	"exit;"
> +	::: __clobber_all);
> +}
> +
> +static __naked __noinline __used long subs_arg6_arg7(long a, long b, long c, long d, long e,
> +						     long f, long g)
> +{
> +	asm volatile (
> +	"r0 = *(u64 *)(r11 + 8);"
> +	"r1 = *(u64 *)(r11 + 16);"
> +	"r0 -= r1;"
> +	"exit;"
> +	::: __clobber_all);
> +}
> +
> +SEC("tc")
> +__description("stack_arg: callx of one of two functions, args are not written")
> +__failure __msg("callee expects 7 args, stack arg1 is not initialized")
> +__naked void callx_one_of_two_not_written(void)
> +{
> +	asm volatile (
> +	".pushsection .rodata,\"a\",@progbits;"
> +	".balign 8;"
> +"two_funcs_%=:"
> +	".quad adds_arg6_arg7;"
> +	".quad subs_arg6_arg7;"
> +	".type two_funcs_%=, @object;"
> +	".size two_funcs_%=, .-two_funcs_%=;"
> +	".popsection;"
> +	"call %[bpf_get_prandom_u32];"
> +	"r0 &= 1;"
> +	"r0 <<= 3;"
> +	"r2 = two_funcs_%= ll;"
> +	"r2 += r0;"
> +	"r6 = *(u64 *)(r2 + 0);"
> +	"r1 = 1;"
> +	"r2 = 2;"
> +	"r3 = 3;"
> +	"r4 = 4;"
> +	"r5 = 5;"
> +	"callx r6;"
> +	"exit;"
> +	:
> +	: __imm(bpf_get_prandom_u32)
> +	: __clobber_all);
> +}

[Severity: Medium]
As reported by Sashiko on v2 [1], should callx_one_of_two_not_written() be
marked with __load_if_JITed() (and guarded with #ifdef CAN_USE_CALLX)?

In tools/testing/selftests/bpf/prog_tests/verifier.c,
test_verifier_stack_arg() runs the suite via RUN(verifier_stack_arg) rather
than RUN_JITED(verifier_stack_arg):

    void test_verifier_stack_arg(void)            { RUN(verifier_stack_arg); }

When the BPF JIT is disabled (for example, bpf_jit_enable=0 when
CONFIG_BPF_JIT_ALWAYS_ON is not set), subtests in verifier_stack_arg.c
without __load_if_JITed() are still loaded.

During verification of callx_one_of_two_not_written(), reading the function
pointer from .rodata and executing callx r6 both invoke
require_callx_jit(env) before reaching check_func_callx_set() and
check_outgoing_stack_args():

    do_check_all()
      -> check_map_mem_read()
        -> check_func_ptr_read()
          -> require_callx_jit()

and:

    do_check_all()
      -> check_func_callx()
        -> require_callx_jit()

In kernel/bpf/verifier.c:require_callx_jit():

    static int require_callx_jit(struct bpf_verifier_env *env)
    {
            if (!env->prog->jit_requested) {
                    verbose(env, "JIT is required to use callx\n");
                    return -EOPNOTSUPP;
            }

Without JIT enabled, wouldn't the verifier reject this program with "JIT is
required to use callx" instead of reaching the expected "callee expects 7
args, stack arg1 is not initialized" error?

[1] https://lore.kernel.org/r/20261105204513.2230344-36-alexei.starovoitov@gmail.com

-- 
Sashiko AI review · https://sashiko.dev/#/patchset/20261106035111.3315285-1-alexei.starovoitov@gmail.com?part=45