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:
