On Wed, Dec 6, 2023 at 8:58 AM Andrei Matei <andreimatei1@xxxxxxxxx> wrote: > > This patch promotes the arithmetic around checking stack bounds to be > done in the 64-bit domain, instead of the current 32bit. The arithmetic > implies adding together a 64-bit register with a int offset. The > register was checked to be below 1<<29 when it was variable, but not > when it was fixed. The offset either comes from an instruction (in which > case it is 16 bit), from another register (in which case the caller > checked it to be below 1<<29 [1]), or from the size of an argument to a > kfunc (in which case it can be a u32 [2]). Between the register being > inconsistently checked to be below 1<<29, and the offset being up to an > u32, it appears that we were open to overflowing the `int`s which were > currently used for arithmetic. > > [1] https://github.com/torvalds/linux/blob/815fb87b753055df2d9e50f6cd80eb10235fe3e9/kernel/bpf/verifier.c#L7494-L7498 > [2] https://github.com/torvalds/linux/blob/815fb87b753055df2d9e50f6cd80eb10235fe3e9/kernel/bpf/verifier.c#L11904 > > Signed-off-by: Andrei Matei <andreimatei1@xxxxxxxxx> > Reported-by: Andrii Nakryiko <andrii.nakryiko@xxxxxxxxx> > --- > kernel/bpf/verifier.c | 6 +++--- > 1 file changed, 3 insertions(+), 3 deletions(-) > LGTM Acked-by: Andrii Nakryiko <andrii@xxxxxxxxxx> > diff --git a/kernel/bpf/verifier.c b/kernel/bpf/verifier.c > index 137240681fa9..6832ed743765 100644 > --- a/kernel/bpf/verifier.c > +++ b/kernel/bpf/verifier.c > @@ -6577,7 +6577,7 @@ 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, > +static int check_stack_slot_within_bounds(s64 off, > struct bpf_func_state *state, > enum bpf_access_type t) > { > @@ -6606,7 +6606,7 @@ static int check_stack_access_within_bounds( > struct bpf_reg_state *regs = cur_regs(env); > struct bpf_reg_state *reg = regs + regno; > struct bpf_func_state *state = func(env, reg); > - int min_off, max_off; > + s64 min_off, max_off; > int err; > char *err_extra; > > @@ -6619,7 +6619,7 @@ static int check_stack_access_within_bounds( > err_extra = " write to"; > > if (tnum_is_const(reg->var_off)) { > - min_off = reg->var_off.value + off; > + min_off = (s64)reg->var_off.value + off; > max_off = min_off + access_size; > } else { > if (reg->smax_value >= BPF_MAX_VAR_OFF || > -- > 2.39.2 >
