Eduard Zingerman writes:
> On Wed, 2024-04-17 at 13:23 +0100, Cupertino Miranda wrote:
>
> [...]
>
>> +SEC("socket")
>> +__description("bounds check for reg32 <= 1, 0 xor (0,1)")
>> +__success __failure_unpriv
>> +__msg_unpriv("R0 min value is outside of the allowed memory range")
>> +__retval(0)
>> +__naked void t_0_xor_01(void)
>> +{
>> + asm volatile (" \
>> + call %[bpf_get_prandom_u32]; \
>> + r6 = r0; \
>> + r1 = 0; \
>> + *(u64*)(r10 - 8) = r1; \
>> + r2 = r10; \
>> + r2 += -8; \
>> + r1 = %[map_hash_8b] ll; \
>> + call %[bpf_map_lookup_elem]; \
>> + if r0 != 0 goto l0_%=; \
>> + exit; \
>> +l0_%=: w1 = 0; \
>> + r6 >>= 63; \
>> + w1 ^= w6; \
>> + if w1 <= 1 goto l1_%=; \
>> + r0 = *(u64*)(r0 + 8); \
>> +l1_%=: r0 = 0; \
>> + exit; \
>> +" :
>> + : __imm(bpf_map_lookup_elem),
>> + __imm_addr(map_hash_8b),
>> + __imm(bpf_get_prandom_u32)
>> + : __clobber_all);
>> +}
>> +
>
> I think that this test case (and one below) should be simplified,
> e.g. as follows:
>
> SEC("socket")
> __success __log_level(2)
> __msg("5: (af) r0 ^= r6 ; R0_w=scalar(smin=smin32=0,smax=umax=smax32=umax32=255,var_off=(0x0; 0xff))")
> __naked void non_const_xor_src_dst(void)
> {
> asm volatile (" \
> call %[bpf_get_prandom_u32]; \
> r6 = r0; \
> call %[bpf_get_prandom_u32]; \
> r6 &= 0xff; \
> r0 &= 0x0f; \
> r0 ^= r6; \
> exit; \
> " :
> : __imm(bpf_map_lookup_elem),
> __imm_addr(map_hash_8b),
> __imm(bpf_get_prandom_u32)
> : __clobber_all);
> }
>
> Patch #2 allows verifier to compute dst range for xor operation with
> non-constant src and dst registers, which is exactly what checked when
> verifier log for instruction "r0 ^= r6" is verified.
> Manipulations with maps, unpriv behavior and retval are just a distraction.
Thanks for the suggestion.
I could not make it fail in the past without that control-flow in the
end of the test. I will try this.
