Daniel Borkmann <dan...@iogearbox.net> [Wed, 2019-04-03 16:19 -0700]:
> On 04/03/2019 11:57 PM, Andrey Ignatov wrote:
> > Daniel Borkmann <dan...@iogearbox.net> [Wed, 2019-04-03 09:46 -0700]:
> >> On 04/03/2019 06:21 PM, Daniel Borkmann wrote:
> >>> On 04/02/2019 10:19 PM, Andrey Ignatov wrote:
...
> >>>
> >>>> min_off = reg->smin_value + reg->off;
> >>>> max_off = reg->umax_value + reg->off;
> >>>> err = __check_stack_boundary(env, regno, min_off,
> >>>> access_size,
> >>
> >> Btw, shouldn't above two additions be sanity checked for wrap-around
> >> resp. truncation?
> >
> > Good question.
> >
> > As I can see, both reg->smin_value and reg->off are checked by
> > check_reg_sane_offset() in adjust_ptr_min_max_vals() that handles
> > pointer arithmetics. And I don't know how to come up with variable
> > offset w/o pointer arithmetics, i.e. these both should be in
> > (-BPF_MAX_VAR_OFF; BPF_MAX_VAR_OFF).
> >
> > As for reg->umax_value, I see that it's checked in check_func_arg()
> > before calling to check_helper_mem_access() (that in turn calls to
> > check_stack_boundary()):
> >
> > if (reg->umax_value >= BPF_MAX_VAR_SIZ) {
> > verbose(env, "R%d unbounded memory access, use 'var &=
> > const' or 'if (var < const)'\n",
> > regno);
> > return -EACCES;
> > }
> >
> > So my understanding is with all these checks that happen beforehand,
> > there should not be overflow and int is used for offset in both the old
> > code, that handles constant offset, and this new code for variable
> > offset.
>
> The latter one is on the reg with size argument, not on the reg with pointer
> to stack. check_helper_mem_access() calls 'regno - 1' for the one where the
> register holds the pointer to stack value.
You're right of course. I saw how size argument is handled and that
'regno - 1', but for some reason missed it while writing previous
answer.
I was able to write a program that exploits max_off overflow, so yeah,
it is a problem. I'll fix it and send v3. Thanks for catching all these
tricky things!
--
Andrey Ignatov
