Re: [PATCH bpf-next v5 8/9] selftests/bpf: Add test to verify xlated insns for global percpu data

[sashiko-bot@kernel.org]

Thank you for your contribution! Sashiko AI review found 2 potential issue(s) to consider:
- [Low] Validation logic for the `insns` array uses the wrong `idx` variable, rendering the bounds check logically ineffective.
- [Low] The `ld_imm64_to_u64()` helper incorrectly casts a signed 32-bit integer to `__u64`, causing unintended sign extension.
--

commit 7b07fe4e4f25a49af6769218dfb2293981c5c2b2
Author: Leon Hwang <leon.hwang@linux.dev>

selftests/bpf: Add test to verify xlated insns for global percpu data
    
This patch adds a test to verify global percpu data related translated
instructions in BPF. It compares the translated ld_imm64 instruction with
the one in the ELF object file, and verifies the addition of the
mov64_percpu_reg instruction.

> diff --git a/tools/testing/selftests/bpf/prog_tests/global_data_init.c b/tools/testing/selftests/bpf/prog_tests/global_data_init.c
> index 729878d5d3b31..c3b9423246b55 100644
> --- a/tools/testing/selftests/bpf/prog_tests/global_data_init.c
> +++ b/tools/testing/selftests/bpf/prog_tests/global_data_init.c
[ ... ]
> +static __u64 ld_imm64_to_u64(const struct bpf_insn *insn)
> +{
> +	return ((__u64) insn[1].imm << 32) | (__u64) insn[0].imm;

[Severity: Low]
Does this code inadvertently sign-extend the lower 32 bits?

The imm field in struct bpf_insn is a signed 32-bit integer (__s32). Casting
it directly to __u64 performs sign extension. If insn[0].imm were to contain
a value with its most significant bit set, it would be sign-extended,
corrupting the resulting 64-bit value.

Should this cast to __u32 first before casting to __u64, like
(__u64)(__u32)insn[0].imm?

> +}
> +
> +static void test_global_percpu_data_xlated(void)
> +{
> +	struct bpf_insn ld_imm64_raw[2], ld_imm64_xlated[2], mov64_percpu_reg, *insns = NULL;
[ ... ]
> +	prog = skel->progs.verifier_percpu_read;
> +	idx = find_ld_imm64(bpf_program__insns(prog), bpf_program__insn_cnt(prog), ld_imm64_raw);
> +	if (!ASSERT_GE(idx, 0, "find_ld_imm64 raw"))
> +		goto out;
> +
> +	err = test_global_percpu_data__load(skel);
> +	if (!ASSERT_OK(err, "test_global_percpu_data__load"))
> +		goto out;
> +
> +	err = get_xlated_program(bpf_program__fd(prog), &insns, &cnt);
> +	if (!ASSERT_OK(err, "get_xlated_program"))
> +		goto out;
> +	if (!ASSERT_GT(cnt, idx + 2, "xlated insn count"))
> +		goto out;

[Severity: Low]
Is this bounds check using the correct index variable?

At this point, idx holds the index from the raw BPF program. Because the
BPF verifier can insert instructions, the index in the translated program
might differ from the raw index.

Could this bounds check be moved after idx is updated below, since
&insns[idx + 2] is accessed later using the translated index?

> +
> +	idx = find_ld_imm64(insns, cnt, ld_imm64_xlated);
> +	if (!ASSERT_GE(idx, 0, "find_ld_imm64 xlated"))
> +		goto out;
> +
> +	ASSERT_EQ(ld_imm64_xlated[0].code, ld_imm64_raw[0].code, "ld_imm64 opcode");
> +	ASSERT_TRUE(ld_imm64_xlated[0].dst_reg == ld_imm64_raw[0].dst_reg, "ld_imm64 dst_reg");

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