On Sat, Dec 2, 2023 at 3:07 PM Andrei Matei <andreimatei1@xxxxxxxxx> wrote:
>
> Push the rounding up of stack offsets into the function responsible for
> growing the stack, rather than relying on all the callers to do it.
> Uncertainty about whether the callers did it or not tripped up people in
> a previous review.
> ---
> kernel/bpf/verifier.c | 8 +++++++-
> 1 file changed, 7 insertions(+), 1 deletion(-)
>
> diff --git a/kernel/bpf/verifier.c b/kernel/bpf/verifier.c
> index bdef4e981dc0..5417c5ad3d88 100644
> --- a/kernel/bpf/verifier.c
> +++ b/kernel/bpf/verifier.c
> @@ -1690,6 +1690,9 @@ static int resize_reference_state(struct bpf_func_state *state, size_t n)
> */
> static int grow_stack_state(struct bpf_verifier_env *env, struct bpf_func_state *state, int size)
> {
> + // The stack size is always a multiple of BPF_REG_SIZE.
let's not use C++-style comments
> + size = round_up(size, BPF_REG_SIZE);
> +
C89 style doesn't allow variable declarations intermixed with code, so
you'll have to do this after declaring variables
> size_t old_n = state->allocated_stack / BPF_REG_SIZE, n = size / BPF_REG_SIZE;
>
> if (old_n >= n)
> @@ -6828,7 +6831,10 @@ static int check_stack_access_within_bounds(
> return err;
> }
>
> - return grow_stack_state(env, state, round_up(-min_off, BPF_REG_SIZE));
> + /* Note that there is no stack access with offset zero, so the needed stack
> + * size is -min_off, not -min_off+1.
> + */
> + return grow_stack_state(env, state, -min_off /* size */);
hmm.. there is still a grow_stack_state() call in
check_stack_write_fixed_off(), right? Which is not necessary because
we do check_stack_access_within_bounds() before that one. Can you drop
it as part of patch #2?
> }
>
> /* check whether memory at (regno + off) is accessible for t = (read | write)
> --
> 2.40.1
>
