On Fri, Dec 8, 2023 at 9:46 PM Kumar Kartikeya Dwivedi <memxor@xxxxxxxxx> wrote:
>
> On Fri, 8 Dec 2023 at 23:15, Andrii Nakryiko <andrii.nakryiko@xxxxxxxxx> wrote:
> >
> > On Thu, Dec 7, 2023 at 7:23 PM Andrei Matei <andreimatei1@xxxxxxxxx> wrote:
> > >
> > > [...]
> > >
> > > > >
> > > > > Ack. Still, if you don't mind entertaining me further, two more questions:
> > > > >
> > > > > 1. What do you make of the code in check_mem_size_reg() [1] where we do
> > > > >
> > > > > if (reg->umin_value == 0) {
> > > > > err = check_helper_mem_access(env, regno - 1, 0,
> > > > > zero_size_allowed,
> > > > > meta);
> > > > >
> > > > > followed by
> > > > >
> > > > > err = check_helper_mem_access(env, regno - 1,
> > > > > reg->umax_value,
> > > > > zero_size_allowed, meta);
> > > > >
> > > > > [1] https://github.com/torvalds/linux/blob/bee0e7762ad2c6025b9f5245c040fcc36ef2bde8/kernel/bpf/verifier.c#L7486-L7489
> > > > >
> > > > > What's the point of the first check_helper_mem_access() call - the
> > > > > zero-sized one
> > > > > (given that we also have the second, broader, check)? Could it be
> > > > > simply replaced by a
> > > > >
> > > > > if (reg->umin_value == 0 && !zero_sized_allowed)
> > > > > err = no_bueno;
> > > > >
> > > >
> > > > Maybe Kumar (cc'ed) can chime in as well, but I suspect that's exactly
> > > > this, and kind of similar to the min_off/max_off discussion we had. So
> > > > yes, I suspect the above simple and straightforward check would be
> > > > much more meaningful and targeted.
> > >
> > > I plan on trying this in a bit; sounds like you're encouraging it.
> > >
> > > >
> > > > I gotta say that the reg->smin_value < 0 check is confusing, though,
> > > > I'm not sure why we are mixing smin and umin/umax in this change...
> > >
> > > When you say "in this change", you mean in the existing code, yeah? I'm not
> >
> > Yeah, sorry, words are hard. It's clearly a question about pre-existing code.
> >
> > > familiar enough with the smin/umin tracking to tell if `reg->smin_value >= 0`
> > > (the condition that the function tests first) implies that
> > > `reg->smin_value == reg->umin_value` (i.e. the fact that we're currently mixing
> >
> > this is probably true most of the time, but knowing how tricky this
> > range tracking is, there is non-zero chance that this is not always
> > true. Which is why I'm a bit confused why we are freely intermixing
> > signed/unsigned range in this code.
> >
> > > smin/umin in check_mem_size_reg() is confusing, but benign). Is that true? If
> > > so, are you saying that check_mem_size_reg() should exclusively use smin/smax?
> > >
> >
> > I'd like to hear from Kumar on what was the intent before suggesting
> > changing anything.
>
> So this was not originally from me, I just happened to move it around
> when adding support for this to kfuncs into a shared helper (if you
> look at the git blame), it's hard for me to comment on the original
> intent, I would know as much as anyone else.
>
> But to helpful, I digged around a bit and found the original patch
> adding this snippet:
>
> 06c1c049721a ("bpf: allow helpers access to variable memory")
>
> It seems the main reason to add that < 0 check on min value was to
> tell the user in the specific case where a spilled value is reloaded
> from stack that they need to mask it again using bitwise operations,
> because back then a spilled constant when reloaded would become
> unknown, and when passed as a parameter to a helper the program would
> be rejected with a weird error trying to access size greater than the
> user specified in C.
>
> Now this change predates the signed/unsigned distinction, that came in:
>
> b03c9f9fdc37 ("bpf/verifier: track signed and unsigned min/max values")
>
> That changes reg->min_value to reg->smin_value, the < 0 comparison
> only makes sense for that.
> Since then that part of the code has stayed the same.
>
> So I think it would probably be better to just use smin/smax as you
> discussed above upthread, also since the BPF_MAX_VAR_SIZE is INT_MAX,
> so it shouldn't be a problem.
Thank you for spelunking, Kumar!
There were two discussions upthread:
1) whether the 0-size check_helper_mem_access() call in
check_mem_size_reg() can be drastically simplified
2) whether the mixed use of smin with umin/umax makes sense.
It seems that we've come up empty-handed on a good reasoning
for 1). I have a patch that simplifies it AND also improves
error messages as a result. I'm inclined to send it for your
consideration, Andrii, if that's cool, as you also didn't
seem to like the current code.
About 2) -- the current mixing of smin/umin/umax actually
makes sense to me. I'd rationalize the (smin < 0) check as
"does this value *look* like a negative value? If so,
opportunistically give the user a nice error message". Even
if the value did not actually come from a signed variable,
but instead came for a very large unsigned, the program
shouldn't verify anyway, so it's no big deal if the negative
interpretation is erroneous. After that check, the value is
exclusively treated as unsigned, since the size argument for
which it is intended is unsigned.
So, I think you can argue either way for the combinations of
signed/unsigned checks that could be done, but I personally
am not inclined to change the current code.
>
> >
> > >
> > > [...]
