Re: [PATCH bpf-next v2 02/15] selftests/bpf: Add tests for ALU on pointers with CAP_PERFMON

[sashiko-bot@kernel.org]

Thank you for your contribution! Sashiko AI review found 1 potential issue(s) to consider:
- [Medium] bpf/selftests: test logic fails to verify ^= and *= on pointers
--

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

selftests/bpf: Add tests for ALU on pointers with CAP_PERFMON

This commit adds BPF selftests to check that various ALU operations
(&=, |=, ^=, >>=, *=) on a pointer to a map value properly produce a scalar
or are rejected as expected depending on capabilities.

> diff --git a/tools/testing/selftests/bpf/progs/verifier_value_illegal_alu.c b/tools/testing/selftests/bpf/progs/verifier_value_illegal_alu.c
> index 9f669cf85c593..31663338866d3 100644
> --- a/tools/testing/selftests/bpf/progs/verifier_value_illegal_alu.c
> +++ b/tools/testing/selftests/bpf/progs/verifier_value_illegal_alu.c
> @@ -165,6 +165,171 @@ __naked void map_ptr_illegal_alu_op(void)
>  	: __clobber_all);
>  }
>  
> +SEC("socket")
> +__description("tag in the low bit of a pointer, and, shift")
> +__success __retval(0)
> +__failure_unpriv __msg_unpriv("R1 bitwise operator &= on pointer")
> +__naked void ptr_tag_and_shift(void)
> +{
> +	asm volatile ("					\
> +	r2 = r10;					\
> +	r2 += -8;					\
> +	r1 = 0;						\
> +	*(u64*)(r2 + 0) = r1;				\
> +	r1 = %[map_hash_48b] ll;			\
> +	call %[bpf_map_lookup_elem];			\
> +	if r0 == 0 goto l0_%=;				\
> +	r1 = r0;					\
> +	r1 &= 1;					\
> +	r2 = r0;					\
> +	r2 >>= 1;					\
> +	r3 = r0;					\
> +	r3 |= 1;					\
> +	r3 ^= 1;					\

[Severity: Medium]
Does this sequence correctly verify the ^= operation on a pointer?

The preceding |= operation converts the pointer in r3 to a scalar, which
means the ^= operation is subsequently performed on a scalar rather than a
pointer.

Additionally, since this program places multiple unprivileged operations
sequentially, wouldn't the verifier abort at the first failure (r1 &= 1),
leaving the unprivileged rejection of the subsequent operations (>>=, |=, ^=)
completely untested?

[ ... ]

> +SEC("socket")
> +__description("number op= pointer")
> +__success __retval(0)
> +__failure_unpriv __msg_unpriv("R1 pointer arithmetic with *= operator")
> +__naked void number_mul_ptr(void)
> +{
> +	asm volatile ("					\
> +	r2 = r10;					\
> +	r2 += -8;					\
> +	r1 = 0;						\
> +	*(u64*)(r2 + 0) = r1;				\
> +	r1 = %[map_hash_48b] ll;			\
> +	call %[bpf_map_lookup_elem];			\
> +	if r0 == 0 goto l0_%=;				\
> +	r1 = 7;						\
> +	r1 *= r0;					\

[Severity: Medium]
Does this actually test the verifier's handling of pointer *= scalar?

The multiplication is applied to a scalar destination (r1) with a pointer
source (r0). Would it be more accurate to test if applying *= to a pointer
destination correctly downgrades the pointer to a scalar?

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