Re: [PATCH bpf-next 2/2] selftests/bpf: Test the emitted bounds check of the array gen_lookup

[sashiko-bot@kernel.org]

Thank you for your contribution! Sashiko AI review found 5 potential
issues to consider:
- [Medium] selftests/bpf: check_jmp32() fails on bpf_jit_needs_zext()
  architectures
- [Medium] selftests/bpf: verifier elides NULL check in
  load_lookup_prog() for in-range key
- [Low] selftests/bpf: test_array_gen_lookup_jmp32() fails when
  bpf_jit_harden is enabled
- [Low] selftests/bpf: check_jmp32() duplicates get_xlated_program()
  helper
- [Low] selftests/bpf: comment style and missing EOF newline in
  array_gen_lookup_jmp32.c
--

commit 10c7c22ab5fe20010aaf42d0505ab98d777d6a8e
Author: Yuan Chen <chenyuan@kylinos.cn>

selftests/bpf: Test the emitted bounds check of the array gen_lookup

This commit adds a bpf selftest that inspects the translated instructions
of inlined array map lookups for a 32-bit bounds check and verifies in-range
and out-of-range lookups via bpf_prog_test_run_opts().

> diff --git a/tools/testing/selftests/bpf/prog_tests/array_gen_lookup_jmp32.c b/tools/testing/selftests/bpf/prog_tests/array_gen_lookup_jmp32.c
> new file mode 100644
> index 0000000000000..ba8215506534a
> --- /dev/null
> +++ b/tools/testing/selftests/bpf/prog_tests/array_gen_lookup_jmp32.c
> @@ -0,0 +1,204 @@
[ ... ]
> +/* Loads a program that inlines bpf_map_lookup_elem() on @map_fd and
> + * returns 2 (XDP_PASS) when the element is found, 1 (XDP_DROP) when the
> + * lookup returns NULL.
> + */

[Severity: Low]
This isn't a bug, but bpf coding style places the opening /* of a
multi-line comment on its own line, and the end of this file is missing a
trailing newline.

> +static int load_lookup_prog(int map_fd, __u32 key)
> +{
> +	const struct bpf_insn insns[] = {
> +		BPF_LD_MAP_FD(BPF_REG_1, map_fd),
> +		BPF_MOV64_REG(BPF_REG_2, BPF_REG_10),
> +		BPF_ALU64_IMM(BPF_ADD, BPF_REG_2, -8),
> +		BPF_ST_MEM(BPF_W, BPF_REG_2, 0, key),
> +		BPF_EMIT_CALL(BPF_FUNC_map_lookup_elem),
> +		BPF_JMP_IMM(BPF_JEQ, BPF_REG_0, 0, 2),
> +		BPF_MOV64_IMM(BPF_REG_0, 2 /* XDP_PASS */),
> +		BPF_JMP_A(1),
> +		BPF_MOV64_IMM(BPF_REG_0, 1 /* XDP_DROP */),
> +		BPF_EXIT_INSN(),
> +	};

[Severity: Medium]
Does the verifier dead-code-eliminate the null check here for in-range
array and per-cpu array keys?

Because key is stored with a constant-immediate store at an 8-byte-aligned
stack offset (r10 - 8), check_stack_write_fixed_off() tracks it as a
spilled constant scalar.  When key < max_entries, check_helper_call() sees
can_elide_value_nullness() and marks r0 as non-null:

kernel/bpf/verifier.c:check_helper_call() {
    ...
		if (func_id == BPF_FUNC_map_lookup_elem &&
		    can_elide_value_nullness(meta.map.ptr) &&
		    meta.max_map_key < meta.map.ptr->max_entries)
			ret_flag &= ~PTR_MAYBE_NULL;
    ...
}

Dead-code elimination then strips the conditional jump and the drop branch
before bpf_do_misc_fixups() inlines the lookup, so the translated program
ignores r0 from the inlined lookup and unconditionally sets r0 = 2.
Storing the key at r10 - 4 or reading the returned element pointer would
keep the runtime check intact.

[ ... ]
> +static void check_jmp32(int prog_fd, __u32 max_entries, const char *what)
> +{
> +	struct bpf_prog_info info = {};
> +	__u32 info_len = sizeof(info), len;
> +	struct bpf_insn *xlated;
> +	__u32 i, jge32 = 0, jge64 = 0;
> +	int err;
> +
> +	err = bpf_prog_get_info_by_fd(prog_fd, &info, &info_len);
> +	if (!ASSERT_OK(err, "prog_get_info"))
> +		return;
> +
> +	len = info.xlated_prog_len;
> +	xlated = calloc(1, len);
> +	if (!ASSERT_OK_PTR(xlated, "calloc xlated"))
> +		return;
> +
> +	memset(&info, 0, sizeof(info));
> +	info.xlated_prog_len = len;
> +	info.xlated_prog_insns = ptr_to_u64(xlated);
> +	err = bpf_prog_get_info_by_fd(prog_fd, &info, &info_len);
> +	if (!ASSERT_OK(err, "prog_get_info xlated"))
> +		goto out;

[Severity: Low]
This isn't a bug, but could check_jmp32() use get_xlated_program() from
testing_helpers.h instead of open-coding the two bpf_prog_get_info_by_fd()
calls and allocation?  Also, test_array_gen_lookup_jmp32() loads the
program with key = 1 to run check_jmp32(), closes prog_fd, and then calls
run_lookup() which loads the exact same program a second time.

> +
> +	for (i = 0; i + 1 < info.xlated_prog_len / (u32)sizeof(*xlated); i++) {
> +		if (xlated[i].code != (BPF_LDX | BPF_W | BPF_MEM))
> +			continue;
> +		if (xlated[i + 1].code != (BPF_JMP32 | BPF_JGE | BPF_K) &&
> +		    xlated[i + 1].code != (BPF_JMP | BPF_JGE | BPF_K))
> +			continue;

[Severity: Medium]
Will check_jmp32() miss the bounds-check jump on 64-bit architectures where
bpf_jit_needs_zext() is true (such as s390x, riscv64, and powerpc64)?

When the patch list from map_gen_lookup() is applied, patch_list_adj_aux()
marks the 32-bit index load with zext_dst = true.  In
bpf_opt_subreg_zext_lo32_rnd_hi32(), architectures with
bpf_jit_needs_zext() insert a zero-extension instruction right after the
load at xlated[i + 1], which shifts the bounds-check jump to xlated[i + 2]
and leaves jge32 at 0.

[ ... ]
> +void test_array_gen_lookup_jmp32(void)
> +{

[Severity: Low]
Can check_jmp32() fail when /proc/sys/net/core/bpf_jit_harden is enabled
on the host or toggled by another test running in parallel under
test_progs -j?

When bpf_jit_harden is 2, bpf_jit_blind_insn() rewrites the immediate
bounds-check jump into register-based instructions using the auxiliary
register before exposing the translated instructions, so jge32 stays 0.
Other tests that inspect translated instructions (such as
serial_test_patch_list()) use the serial_test_ prefix and set
/proc/sys/net/core/bpf_jit_harden to 0 while loading the program.

-- 
Sashiko AI review · https://sashiko.dev/#/patchset/20261115102559.465375-1-chenyuan@kylinos.cn?part=2