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. > > > > > [...]