[...]
> >
> > diff --git a/kernel/bpf/verifier.c b/kernel/bpf/verifier.c
> > index af2819d5c8ee..b646bdde09cd 100644
> > --- a/kernel/bpf/verifier.c
> > +++ b/kernel/bpf/verifier.c
> > @@ -6816,10 +6816,9 @@ static int check_stack_access_within_bounds(
> > return -EACCES;
> > }
> > min_off = reg->smin_value + off;
> > + max_off = reg->smax_value + off;
> > if (access_size > 0)
> > - max_off = reg->smax_value + off + access_size - 1;
> > - else
> > - max_off = min_off;
> > + max_off += access_size - 1;
>
> this special casing of access_size == 0 feels wrong (and I mean before
> your patch as well).
>
> Looking at the code, we only really calculate max_off to check that we
> don't go to a non-negative stack offset, e.g., r10+0 or r10+1 (and
> beyond).
>
> So given that, I propose to calculate max_off as an exclusive bound,
> and instead of doing a mostly useless check_stack_slot_within_bounds()
> call for it, just check that max_off is <= 0.
>
> Something like this:
>
> min_off = reg->smin_value + off;
> max_off = reg->smax_value + off + access_size;
> err = check_stack_slot_within_bounds(min_off, state, type);
> if (!err && max_off > 0)
> err = -EINVAL; /* out of stack access into non-negative offsets */
Dealing with access_size == 0 indeed feels dubious to me, but I'm not entirely
sure that your suggested code is better. min_off being inclusive and
max_off being
exclusive seems surprising. I'll do it if you want, I don't care too much.
We could keep max_off exclusive, and still not call
check_stack_slot_within_bounds() for it:
min_off = reg->smin_value + off;
max_off = reg->smax_value + off + access_size - 1;
err = check_stack_slot_within_bounds(min_off, state, type);
if (!err && max_off >= 0)
err = -EINVAL; /* out of stack access into non-negative offsets */
But now max_off can be below min_off, which again seems confusing.
What I'd really like to know is whether this whole zero access_size business
deserves to exist. Do you know what the point of verifying a zero-sized access
is exactly / could we turn 0-byte access into 1-byte accesses and
verify that instead?
Because then there'd be no more special case to consider.
>
>
> Now, one more issue that jumped out at me is that we calculate min/max
> off as a sum of smin/smax values (which are checked to be within
> +/-1<<29, all good so far) *and* insn->off, which can be a full s32,
> it seems. So we are running into overflow/underflow territory with
> using int for min_off/max_off.
>
> While you are at it, can you please use s64 for all these calculations? Thanks!
>
>
> > }
> >
> > err = check_stack_slot_within_bounds(min_off, state, type);
Will do.
