On Tue, Nov 21, 2023 at 1:46 AM Andrii Nakryiko
<andrii.nakryiko@xxxxxxxxx> wrote:
>
>
> It *feels* like this stack depth update *and* growing allocated stack
> slots should happen somewhere in check_stack_access_within_bounds() or
> right after it. It shouldn't matter whether we read or write to the
> stack slot: either way that slot becomes part of the verifier state
> that we should take into account during state comparison. Eduard not
> so long ago added a change that allows reading STACK_INVALID slots, so
> it's completely valid to read something that was never written to (and
> so grow_stack_state() wasn't called for that slot, as it is
> implemented right now). So I think we should fix that.
>
Agree. The following cases are currently confusing to me.
The verifier accepts the following program, which goes from #4 to #8
and directly read the stack at runtime without any previous write:
func#0 @0
0: R1=ctx() R10=fp0
0: (bf) r6 = r10 ; R6_w=fp0 R10=fp0
1: (85) call bpf_get_prandom_u32#7 ; R0_w=scalar()
2: (bf) r3 = r0 ; R0_w=scalar(id=1) R3_w=scalar(id=1)
3: (bf) r8 = r0 ; R0_w=scalar(id=1) R8_w=scalar(id=1)
4: (4e) if w0 & w3 goto pc+3 ; R0_w=scalar(id=1) R3_w=scalar(id=1)
5: (63) *(u32 *)(r6 -196) = r3 ; R3_w=scalar(id=1) R6_w=fp0
fp-200=mmmm????
6: (18) r7 = 0x19 ; R7=25
8: (61) r7 = *(u32 *)(r6 -200) ; R6=fp0
R7_w=scalar(smin=0,smax=umax=0xffffffff,var_off=(0x0; 0xffffffff))
fp-200=mmmm????
9: (95) exit
from 4 to 8: safe
verification time 358 usec
stack depth 200
processed 10 insns (limit 1000000) max_states_per_insn 0 total_states
1 peak_states 1 mark_read 1
The state is pruned, because of this:
static bool stacksafe(...)
....
if (env->allow_uninit_stack &&
old->stack[spi].slot_type[i % BPF_REG_SIZE] == STACK_MISC)
continue;
Yet, the sample direct read would be rejected:
func#0 @0
0: R1=ctx() R10=fp0
0: (bf) r6 = r10 ; R6_w=fp0 R10=fp0
1: (61) r7 = *(u32 *)(r6 -200)
invalid read from stack R6 off=-200 size=4
Eduard, you added support for reading uninit slots, should we also add something
like the following:
diff --git a/kernel/bpf/verifier.c b/kernel/bpf/verifier.c
index 8c2d31aa3d31..aa861d2da240 100644
--- a/kernel/bpf/verifier.c
+++ b/kernel/bpf/verifier.c
@@ -6446,7 +6446,7 @@ static int check_stack_slot_within_bounds(int off,
{
int min_valid_off;
- if (t == BPF_WRITE)
+ if (t == BPF_WRITE || env->allow_uninit_stack)
min_valid_off = -MAX_BPF_STACK;
else
min_valid_off = -state->allocated_stack;
