Re: [PATCH v2 bpf-next 2/4] bpf: Track delta between "linked" registers.

[Eduard Zingerman <eddyz87@gmail.com>]

On Mon, 2024-06-10 at 16:08 -0700, Alexei Starovoitov wrote:

[...]

> diff --git a/include/linux/bpf_verifier.h b/include/linux/bpf_verifier.h
> index 50aa87f8d77f..2b54e25d2364 100644
> --- a/include/linux/bpf_verifier.h
> +++ b/include/linux/bpf_verifier.h
> @@ -73,7 +73,10 @@ enum bpf_iter_state {
>  struct bpf_reg_state {
>  	/* Ordering of fields matters.  See states_equal() */
>  	enum bpf_reg_type type;
> -	/* Fixed part of pointer offset, pointer types only */
> +	/*
> +	 * Fixed part of pointer offset, pointer types only.
> +	 * Or constant delta between "linked" scalars with the same ID.
> +	 */
>  	s32 off;

After thinking about this some more I came to conclusion that ->off
has to be checked for scalar registers in regsafe().
Otherwise the following test is marked as safe:

char buf[10] SEC(".data.buf");

SEC("socket")
__failure
__flag(BPF_F_TEST_STATE_FREQ)
__naked void check_add_const_regsafe_off(void)
{
	asm volatile (
	"r8 = %[buf];"
	"call %[bpf_ktime_get_ns];"
	"r6 = r0;"
	"call %[bpf_ktime_get_ns];"
	"r7 = r0;"
	"call %[bpf_ktime_get_ns];"
	"r1 = r0;"		/* same ids for r1 and r0 */
	"if r6 > r7 goto 1f;"	/* this jump can't be predicted */
	"r1 += 1;"		/* r1.off == +1 */
	"goto 2f;"
	"1: r1 += 100;"		/* r1.off == +100 */
	"goto +0;"		/* force checkpoint, must verify r1.off in regsafe() here */
	"2: if r0 > 8 goto 3f;"	/* r0 range [0,8], r1 range either [1,9] or [100,108]*/
	"r8 += r1;"
	"*(u8 *)(r8 +0) = r0;"	/* potentially unsafe, buf size is 10 */
	"3: exit;"
	:
	: __imm(bpf_ktime_get_ns),
	  __imm_ptr(buf)
	: __clobber_common);
}

Sorry for missing this yesterday.
Something like below is necessary.
(To trigger ((rold->id & BPF_ADD_CONST) != (rcur->id & BPF_ADD_CONST))
 a variation of the test where r1 += 1 is not done is necessary).

---

diff --git a/kernel/bpf/verifier.c b/kernel/bpf/verifier.c
index ad11e5441860..70e44fa4f765 100644
--- a/kernel/bpf/verifier.c
+++ b/kernel/bpf/verifier.c
@@ -16797,6 +16797,10 @@ static bool regsafe(struct bpf_verifier_env *env, struct bpf_reg_state *rold,
                }
                if (!rold->precise && exact == NOT_EXACT)
                        return true;
+               if ((rold->id & BPF_ADD_CONST) != (rcur->id & BPF_ADD_CONST))
+                       return false;
+               if ((rold->id & BPF_ADD_CONST) && (rold->off != rcur->off))
+                       return false;
                /* Why check_ids() for scalar registers?
                 *
                 * Consider the following BPF code: