Re: [PATCH v2 bpf-next 5/6] selftests/bpf: Cover invariant violation cases from syzbot

[Mykyta Yatsenko <mykyta.yatsenko5@gmail.com>]

Paul Chaignon <paul.chaignon@gmail.com> writes:

> This patch adds a selftest for the change in the previous patch. The
> selftest is derived from a syzbot reproducer from [1] (among the 22
> reproducers on that page, only 4 still reproduced on latest bpf tree,
> all being small variants of the same invariant violation).
>
> The test case failure without the previous patch is shown below.
>
>   0: R1=ctx() R10=fp0
>   0: (85) call bpf_get_prandom_u32#7    ; R0=scalar()
>   1: (bf) r5 = r0                       ; R0=scalar(id=1) R5=scalar(id=1)
>   2: (57) r5 &= -4                      ; R5=scalar(smax=0x7ffffffffffffffc,umax=0xfffffffffffffffc,smax32=0x7ffffffc,umax32=0xfffffffc,var_off=(0x0; 0xfffffffffffffffc))
>   3: (bf) r7 = r0                       ; R0=scalar(id=1) R7=scalar(id=1)
>   4: (57) r7 &= 1                       ; R7=scalar(smin=smin32=0,smax=umax=smax32=umax32=1,var_off=(0x0; 0x1))
>   5: (07) r7 += -43                     ; R7=scalar(smin=smin32=-43,smax=smax32=-42,umin=0xffffffffffffffd5,umax=0xffffffffffffffd6,umin32=0xffffffd5,umax32=0xffffffd6,var_off=(0xffffffffffffffd4; 0x3))
>   6: (5e) if w5 != w7 goto pc+1
>   verifier bug: REG INVARIANTS VIOLATION (false_reg1): range bounds violation u64=[0xffffffd5, 0xffffffffffffffd4] s64=[0x80000000ffffffd5, 0x7fffffffffffffd4] u32=[0xffffffd5, 0xffffffd4] s32=[0xffffffd5, 0xffffffd4] var_off=(0xffffffd4, 0xffffffff00000000)
>
> R5 and R7 are prepared such that their tnums intersection results in a
> known constant but that constant isn't within R7's u32 bounds.
> is_branch_taken isn't able to detect this case today, so the verifier
> walks the impossible fallthrough branch. After regs_refine_cond_op and
> reg_bounds_sync refine R5 on the assumption that the branch is taken,
> the impossibility becomes apparent and results in an invariant violation
> for R5: umin32 is greater than umax32.
>
> The previous patch fixes this by using regs_refine_cond_op and
> reg_bounds_sync in is_branch_taken to detect the impossible branch. The
> fallthrough branch is therefore correctly detected as dead code.
>
> Link: https://syzkaller.appspot.com/bug?extid=c950cc277150935cc0b5 [1]
> Signed-off-by: Paul Chaignon <paul.chaignon@gmail.com>
> ---
>  .../selftests/bpf/progs/verifier_bounds.c     | 24 +++++++++++++++++++
>  1 file changed, 24 insertions(+)
>
> diff --git a/tools/testing/selftests/bpf/progs/verifier_bounds.c b/tools/testing/selftests/bpf/progs/verifier_bounds.c
> index 3724d5e5bcb3..818efa08404d 100644
> --- a/tools/testing/selftests/bpf/progs/verifier_bounds.c
> +++ b/tools/testing/selftests/bpf/progs/verifier_bounds.c
> @@ -2070,4 +2070,28 @@ __naked void refinement_32bounds_not_overwriting_64bounds(void *ctx)
>  	: __clobber_all);
>  }
>  
> +/* Last jump can be detected as always taken because the intersection of R5 and
> + * R7 32bit tnums produces a constant that isn't within R7's s32 bounds.
> + */
> +SEC("socket")
> +__description("dead branch: tnums give impossible constant if equal")
> +__success
> +__flag(BPF_F_TEST_REG_INVARIANTS)
> +__naked void tnums_equal_impossible_constant(void *ctx)
> +{
> +	asm volatile("										\
> +	call %[bpf_get_prandom_u32];								\
> +	r5 = r0;										\
> +	r5 &= 0xfffffffffffffffc;	/* var_off32=(0; 0xfffffffc) */				\
> +	r7 = r0;										\
> +	r7 &= 0x1;			/* var_off32=(0x0; 0x1) */				\
> +	r7 += -43;			/* s32=[-43; -42] & var_off32=(0xffffffd4; 0x3) */	\
> +	if w5 != w7 goto +1;		/* on fallthrough var_off32=-44, not in s32 */		\
w5 has bits 0,1 zero, rest unknown
w7 has top bits known as 0xffffffd4, and bits 0,1 unknown
If w5 == w7, their tnums must intersect to a single
value = 0xffffffd4(-44). Which is outside of w7's range [-43; 42]. So
the r10 = 0 branch should be unreachable.
I guess the comment should be updated to the kernel style as well.
Acked-by: Mykyta Yatsenko <yatsenko@meta.com>
> +	r10 = 0;										\
> +	exit;											\
> +"	:
> +	: __imm(bpf_get_prandom_u32)
> +	: __clobber_all);
> +}
> +
>  char _license[] SEC("license") = "GPL";
> -- 
> 2.43.0