On Sun, Dec 10, 2023 at 6:13 PM Kumar Kartikeya Dwivedi
<memxor@xxxxxxxxx> wrote:
>
> On Sun, 10 Dec 2023 at 23:46, Andrei Matei <andreimatei1@xxxxxxxxx> wrote:
> >
> > 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.
> >
>
> While that's true, I think it should probably go into
> check_helper_mem_access instead of being duplicated for handlers of
> each switch statement.
> It seems one of them (for PTR_TO_MAP_KEY) hardcodes it regardless of
> what's passed in, and there's a special case of register_is_null which
> is permitted.
>
> So it might be better to unify the handling in check_helper_mem_access
> instead of its callers. Just my $0.02.
My initial focus is getting check_mem_size_reg() to not call
check_helper_mem_access() twice. That's what patch [1] does.
Then, I tend to agree with you that
check_helper_mem_access() forwarding zero_size_allowed() to
a bunch of switch arms seems unnecessary; it also bothered
me. I did try to do something about it for a bit - terminate
the handling of zero_sized_allowed somewhere - but the thing
is that the utilities used in that switch (e.g.
check_mem_region_access()) are also called in other places,
with both true and false for zero_sized_allowed. So I didn't
immediately come up with something better and gave up for
now. But if you have throughts, let's take it to the new
patch I'd say.
[1] https://lore.kernel.org/bpf/20231210225536.70322-1-andreimatei1@xxxxxxxxx/
>
> > 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.
> >
>
> I think based on the thread it would be better to atleast comments
> explaining all this, even if in the end we don't decide to touch it.
Yeah... But comment what exactly? I could put a comment on
the (smin_value < 0) check saying something "if the value
looks negative, assume that it came from a signed variable
and give a helpful error message", and imply that the value
should be treated as unsigned from that moment on. But then
it gets confusing when, a few lines down,
check_helper_mem_access() takes the size as `int` instead of
`u32`. So the truth I'm not entirely sure what to say, plus
Andrii might have other ideas about how the bike shed should
be colored. If we build more consensus though, I'm all about
adding comments.
>
> >
> > [...]
