On Tue, Nov 21, 2023 at 12:16 PM Hao Sun <sunhao.th@xxxxxxxxx> wrote: > > 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. Like Hao, I'm also confused about when reads from unitialized stack slots are supposed to be allowed and when they're not. For example, the "uninitialized stack1" test ([0]) seems to check that at least some type of uninitialized read is not permitted. The reason why the respective access is rejected and this test currently passes is the following check in check_stack_range_initialized: [1]. We summarily reject the access to slots beyond state->allocated_stack. If we were to not reject them there, and instead treat the slots as STACK_INVALID, then the test's access would be allowed just below, through the combination of env->allow_uninit_stack and `clobber`. So, assuming that this tests (and a few others) are sane, Andrii's suggestion of calling grow_stack_state()/update_stack_depth() in check_stack_access_within_bounds() does not immediately work: doing so would change the behavior in check_stack_range_initialized() and allow the access. On the other hand, perhaps the test is not sane and the access should be permitted, in the spirit of allowing reads of uninitialized stack? Perhaps the different treatment of slots beyond state->allocated_stack and STACK_INVALID slots is not intentional. Though, I am fairly confused about the idea of allowing such reads in general - don't they allow leaking of kernel memory from the uninit stack to user space? Even if restricted to root, is the leak ok? To also state the obvious - I'm happy to continue working on this patch and fix a bug I've caused. However, if someone who actually knows the deal wants to take over, I'm certainly not attached to anything :). [0] https://github.com/torvalds/linux/blob/98b1cc82c4affc16f5598d4fa14b1858671b2263/tools/testing/selftests/bpf/progs/verifier_basic_stack.c#L28-L44 [1] https://github.com/torvalds/linux/blob/98b1cc82c4affc16f5598d4fa14b1858671b2263/kernel/bpf/verifier.c#L7280 > > 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;
