[bpf,v3,3/3] bpf: minor cleanup around stack bounds

Commit Message

Andrei Matei Dec. 2, 2023, 11:05 p.m. UTC

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(-)

Comments

Andrii Nakryiko Dec. 4, 2023, 6:19 p.m. UTC | #1

On Sat, Dec 2, 2023 at 3:07 PM Andrei Matei <[e-mail]> 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
>

Eduard Zingerman Dec. 4, 2023, 6:43 p.m. UTC | #2

On Mon, 2023-12-04 at 10:19 -0800, Andrii Nakryiko wrote:
[...]
> > @@ -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?

I'm not sure I understand what you mean. Patch #2 (v3) drops
grow_stack_state() from check_stack_write_fixed_off()
so all seems good?

Andrii Nakryiko Dec. 4, 2023, 7:06 p.m. UTC | #3

On Mon, Dec 4, 2023 at 10:43 AM Eduard Zingerman <[e-mail]> wrote:
>
> On Mon, 2023-12-04 at 10:19 -0800, Andrii Nakryiko wrote:
> [...]
> > > @@ -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?
>
> I'm not sure I understand what you mean. Patch #2 (v3) drops
> grow_stack_state() from check_stack_write_fixed_off()
> so all seems good?

I swear I checked for that, both by re-reading the patch and by
searching in the browser. And check_stack_write_fixed_off() was
nowhere to be found. Now triple checking that I see that it's Gmail's
smartness that collapsed that portion of the patch (but not the
others!) into a subtle triple dot region, which made all that
invisible and non-searchable, sigh...

Sorry for the noise, all good then.

Patch

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.
+	size = round_up(size, BPF_REG_SIZE);
+
 	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 */);
 }
 
 /* check whether memory at (regno + off) is accessible for t = (read | write)