On Mon, Nov 20, 2023 at 7:46 PM Andrii Nakryiko <andrii.nakryiko@xxxxxxxxx> wrote: > > On Mon, Nov 13, 2023 at 3:51 PM Andrei Matei <andreimatei1@xxxxxxxxx> wrote: > > > > Before this patch, writes to the stack using registers containing a > > variable offset (as opposed to registers with fixed, known values) were > > not properly contributing to the function's needed stack size. As a > > result, it was possible for a program to verify, but then to attempt to > > read out-of-bounds data at runtime because a too small stack had been > > allocated for it. > > > > Each function tracks the size of the stack it needs in > > bpf_subprog_info.stack_depth, which is maintained by > > update_stack_depth(). For regular memory accesses, check_mem_access() > > was calling update_state_depth() but it was passing in only the fixed > > part of the offset register, ignoring the variable offset. This was > > incorrect; the minimum possible value of that register should be used > > instead. > > > > This patch fixes it by pushing down the update_stack_depth() call into > > grow_stack_depth(), which then correctly uses the registers lower bound. > > grow_stack_depth() is responsible for tracking the maximum stack size > > for the current verifier state, so it seems like a good idea to couple > > it with also updating the per-function high-water mark. As a result of > > this re-arrangement, update_stack_depth() is no longer needlessly called > > for reads; it is now called only for writes (plus other cases like > > helper memory access). I think this is a good thing, as reads cannot > > possibly grow the needed stack. > > I'm going to disagree. I think we should calculate max stack size both > on reads and writes. I'm not sure why it's ok for a BPF program to > access a stack with some big offset, but the BPF verifier not > rejecting this. What do I miss? My intention was certainly not to change any verification behavior. I was relying on reads to uninit stack not being allowed regardless of the tracked stacked bounds; I now see that things don't work that way.
