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

[Leon Hwang <leon.hwang@linux.dev>]

On 8/6/26 23:21, sashiko-bot@kernel.org wrote:
[...]
>> +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?
> 

Ack.

Will add (__u32).

>> +}
>> +
>> +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?
> 
Ack.

Will check cnt using the new idx.

Thanks,
Leon