On Sat, Dec 2, 2023 at 3:07 PM Andrei Matei <andreimatei1@xxxxxxxxx> wrote:
>
> Privileged programs are supposed to be able to read uninitialized stack
> memory (ever since 6715df8d5) but, before this patch, these accesses
> were permitted inconsistently. In particular, accesses were permitted
> above state->allocated_stack, but not below it. In other words, if the
> stack was already "large enough", the access was permitted, but
> otherwise the access was rejected instead of being allowed to "grow the
> stack". This undesired rejection was happening in two places:
> - in check_stack_slot_within_bounds()
> - in check_stack_range_initialized()
> This patch arranges for these accesses to be permitted. A bunch of tests
> that were relying on the old rejection had to change; all of them were
> changed to add also run unprivileged, in which case the old behavior
> persists. One tests couldn't be updated - global_func16 - because it
> can't run unprivileged for other reasons.
>
> This patch also fixes the tracking of the stack size for variable-offset
> reads. This second fix is bundled in the same commit as the first one
> because they're inter-related. 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 tracking is now fixed by centralizing the tracking of stack size in
> grow_stack_state(), and by lifting the calls to grow_stack_state() to
> check_stack_access_within_bounds() as suggested by Andrii. The code is
> now simpler and more convincingly tracks the correct maximum stack size.
> check_stack_range_initialized() can now rely on enough stack having been
> allocated for the access; this helps with the fix for the first issue.
>
> A few tests were changed to also check the stack depth computation. The
> one that fails without this patch is verifier_var_off:stack_write_priv_vs_unpriv.
>
> Reported-by: Hao Sun <sunhao.th@xxxxxxxxx>
> Fixes: 01f810ace9ed3 ("bpf: Allow variable-offset stack access")
> Closes: https://lore.kernel.org/bpf/CABWLsev9g8UP_c3a=1qbuZUi20tGoUXoU07FPf-5FLvhOKOY+Q@xxxxxxxxxxxxxx/
> Signed-off-by: Andrei Matei <andreimatei1@xxxxxxxxx>
> ---
> kernel/bpf/verifier.c | 67 ++++++++-----------
> tools/testing/selftests/bpf/progs/iters.c | 2 +-
> .../selftests/bpf/progs/test_global_func16.c | 2 +-
> .../bpf/progs/verifier_basic_stack.c | 8 +--
> .../selftests/bpf/progs/verifier_int_ptr.c | 5 +-
> .../selftests/bpf/progs/verifier_raw_stack.c | 5 +-
> .../selftests/bpf/progs/verifier_var_off.c | 62 ++++++++++++++---
> .../selftests/bpf/verifier/atomic_cmpxchg.c | 11 ---
> tools/testing/selftests/bpf/verifier/calls.c | 4 +-
> 9 files changed, 93 insertions(+), 73 deletions(-)
>
LGTM, thanks! (and yes, changing prog type for test is fine)
Acked-by: Andrii Nakryiko <andrii@xxxxxxxxxx>
> diff --git a/kernel/bpf/verifier.c b/kernel/bpf/verifier.c
> index af2819d5c8ee..bdef4e981dc0 100644
> --- a/kernel/bpf/verifier.c
> +++ b/kernel/bpf/verifier.c
> @@ -1685,7 +1685,10 @@ static int resize_reference_state(struct bpf_func_state *state, size_t n)
> return 0;
> }
>
[...]
