On Sat, Nov 25, 2023 at 5:53 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.
>
> 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.
>
> 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>
> ---
> include/linux/bpf_verifier.h | 4 ++
> kernel/bpf/verifier.c | 70 ++++++++-----------
> .../selftests/bpf/progs/test_global_func16.c | 2 +-
> .../bpf/progs/verifier_basic_stack.c | 6 +-
> .../selftests/bpf/progs/verifier_int_ptr.c | 2 +-
> .../selftests/bpf/progs/verifier_raw_stack.c | 2 +-
> .../selftests/bpf/progs/verifier_var_off.c | 4 +-
> .../selftests/bpf/verifier/atomic_cmpxchg.c | 11 ---
> tools/testing/selftests/bpf/verifier/calls.c | 2 +-
> 9 files changed, 42 insertions(+), 61 deletions(-)
>
> diff --git a/include/linux/bpf_verifier.h b/include/linux/bpf_verifier.h
> index aa4d19d0bc94..5fc389e8be35 100644
> --- a/include/linux/bpf_verifier.h
> +++ b/include/linux/bpf_verifier.h
> @@ -630,6 +630,10 @@ struct bpf_verifier_env {
> int exception_callback_subprog;
> bool explore_alu_limits;
> bool allow_ptr_leaks;
> + /* Allow access to uninitialized stack memory. Writes with fixed offset are
> + * always allowed, so this refers to reads (with fixed or variable offset),
> + * to writes with variable offset and to indirect (helper) accesses.
> + */
> bool allow_uninit_stack;
> bool bpf_capable;
> bool bypass_spec_v1;
> diff --git a/kernel/bpf/verifier.c b/kernel/bpf/verifier.c
> index af2819d5c8ee..f9546dd73f3c 100644
> --- a/kernel/bpf/verifier.c
> +++ b/kernel/bpf/verifier.c
> @@ -1685,10 +1685,12 @@ static int resize_reference_state(struct bpf_func_state *state, size_t n)
> return 0;
> }
>
> -static int grow_stack_state(struct bpf_func_state *state, int size)
> +/* Possibly update state->allocated_stack to be at least size bytes. Also
> + * possibly update the function's high-water mark in its bpf_subprog_info.
> + */
> +static int grow_stack_state(struct bpf_verifier_env *env, struct bpf_func_state *state, int size)
> {
> size_t old_n = state->allocated_stack / BPF_REG_SIZE, n = size / BPF_REG_SIZE;
shouldn't this be rounding up? (size + BPF_REG_SIZE - 1) / BPF_REG_SIZE?
> -
According to kernel code style, there should be an empty line between
variable declaration and other statements. Please keep this empty
line.
> if (old_n >= n)
> return 0;
>
[...]
> @@ -6761,13 +6748,15 @@ static int check_ptr_to_map_access(struct bpf_verifier_env *env,
> * The minimum valid offset is -MAX_BPF_STACK for writes, and
> * -state->allocated_stack for reads.
> */
> -static int check_stack_slot_within_bounds(int off,
> - struct bpf_func_state *state,
> - enum bpf_access_type t)
> +static int check_stack_slot_within_bounds(
> + struct bpf_verifier_env *env,
> + int off,
> + struct bpf_func_state *state,
> + enum bpf_access_type t)
nit: please keep code style, first argument is normally on the same
line as opening parenthesis
> {
> 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;
[...]
> diff --git a/tools/testing/selftests/bpf/progs/verifier_var_off.c b/tools/testing/selftests/bpf/progs/verifier_var_off.c
> index 83a90afba785..bbf3628c625a 100644
> --- a/tools/testing/selftests/bpf/progs/verifier_var_off.c
> +++ b/tools/testing/selftests/bpf/progs/verifier_var_off.c
> @@ -61,7 +61,7 @@ __naked void stack_read_priv_vs_unpriv(void)
>
> SEC("lwt_in")
> __description("variable-offset stack read, uninitialized")
> -__failure __msg("invalid variable-offset read from stack R2")
> +__success
> __naked void variable_offset_stack_read_uninitialized(void)
> {
> asm volatile (" \
> @@ -255,7 +255,7 @@ __naked void access_min_out_of_bound(void)
>
> SEC("lwt_in")
> __description("indirect variable-offset stack access, min_off < min_initialized")
> -__failure __msg("invalid indirect read from stack R2 var_off")
> +__success
as Eduard mentioned, can you please try updating program type to
something that is allowed in unprivileged mode, e.g., SEC("socket")
and make sure that it still fails in unpriv mode
> __naked void access_min_off_min_initialized(void)
> {
> asm volatile (" \
[...]
