On Wed, Jan 04, 2023 at 03:03:23PM -0800, Andrii Nakryiko wrote: > On Wed, Jan 4, 2023 at 2:35 PM Alexei Starovoitov > <alexei.starovoitov@xxxxxxxxx> wrote: > > > > On Tue, Jan 03, 2023 at 02:04:44PM -0800, Andrii Nakryiko wrote: > > > > It sounds logical, but it can get tricky with ranges and branch taken logic. > > > > Consider something like: > > > > R1=(min=2,max=8), R2=(min=1, max=10) > > > > if (R1 within R2) // bpf prog is doing its own 'within' > > > > > > a bit confused what is "R1 within R2" here and what you mean "bpf prog > > > is doing its own 'within'"? Any sort of `R1 < R2` checks (and any > > > other op: <=, >=, etc) can't really kick in branch elimination because > > > R2_min=1 < R1_max=8, so arithmetically speaking we can't conclude that > > > "R1 is always smaller than R2", so both branches would have to be > > > examined. > > > > Something like that. Details didn't matter to me. > > It was hypothetical 'within' operation just to illustrate the point. > > I just don't know what kind of instruction has this "within" > semantics, that's why I was confused. > > > > > > But I probably misunderstood your example, sorry. > > > > > > > // branch taken kicks in > > > > else > > > > // issues that were never checked > > > > > > > > Now new state has: > > > > R1=(min=4,max=6), R2=(min=5, max=5) > > > > > > > > Both R1 and R2 of new state individually range_within of old safe state, > > > > but together the prog may go to the unverified path. > > > > Not sure whether it's practical today. > > > > You asked for hypothetical, so here it goes :) > > > > > > No problem with "hypothetical-ness". But my confusion and argument is > > > similarly "in principle"-like. Because if such an example above can be > > > constructed then this would be an issue for SCALAR as well, right? And > > > if you can bypass verifier's safety with SCALAR, you (hypothetically) > > > could use that SCALAR to do out-of-bounds memory access by adding this > > > SCALAR to some mem-like register. > > > > Correct. The issue would apply to regular scalar if such 'within' operation > > was available. > > > > > So that's my point and my source of confusion: if we don't trust > > > var_off+range_within() logic to handle *all* situations correctly, > > > then we should be worried about SCALARs just as much as anything else > > > (unless, as usual, I missed something). > > > > Yes. I personally don't believe that doing range_within for all regtypes > > by default is a safer way forward. > > The example wasn't real. It was trying to demonstrate a possible issue. > > You insist to see a real example with range_within. > > I don't have it. It's a gut feel that it could be there because > > I could construct it with fake 'within'. > > Ok, so some new instruction with "within" semantics would be > necessary. I was just trying to see if I'm missing some existing > potential case. Seems like not, that's fine. > > > > > > > More gut feel than real issue. > > > > > > > > > > > > > > > > > > > > > SCALARS and PTR_TO_BTF_ID will likely dominate future bpf progs. > > > > > > Keeping default as regs_exact (that does ID match) is safer default. > > > > > > > > > > It's fine, though the point of this patch set was patch #7, enabling > > > > > logic similar to PTR_TO_MAP_VALUE for PTR_TO_MEM and PTR_TO_BUF. I can > > > > > send specific fixes for that, no problem. But as I said above, I'm > > > > > really curious to understand what kind of situations will lead to > > > > > unsafety if we do var_off+range_within checks. > > > > > > > > PTR_TO_MEM and PTR_TO_BUF explicitly are likely ok despite my convoluted > > > > example above. > > > > I'm less sure about PTR_TO_BTF_ID. It could be ok. > > > > Just feels safer to opt-in each type explicitly. > > > > > > Sure, I can just do a simple opt-in, no problem. As I said, mostly > > > trying to understand the issue overall. > > > > > > For PTR_TO_BTF_ID specifically, I can see how we can enable > > > var_off+range_within for cases when we access some array, right? But > > > then I think we'll be enforcing that we are staying within the > > > boundaries of a single array field, never crossing into another field. > > > > Likely yes, but why? > > No reason, just seems sane. But it also doesn't matter for the > discussion at hand. > > > You're trying hard to collapse the switch statement in regsafe() > > while claiming it's a safer way. I don't see it this way. > > For example the upcoming active_lock_id would need its own check_ids() call. > > It will be necessary for PTR_TO_BTF_ID only. > > Why collapse the switch into 'default:' just to bring some back? > > The default without checking active_lock_id through check_ids > > would be wrong, so collapsed switch doesn't make things safer. > > I'm saying it's sane default and is better than what we have today. > The reason I want(ed) to make default case doing proper range checks > (if they are set) is so that we don't miss cases like PTR_TO_MEM and > PTR_TO_BUF in the future. Wait. What do you mean 'miss cases like PTR_TO_MEM' ? With 'switch() default: regs_exact()' it's not a safety issue that PTR_TO_MEM doesn't do range_within() like PTR_TO_MAP_KEY. The verifier is unnecessary conservative with PTR_TO_MEM. Applying range_within() will allow more valid programs to be accepted. What did I miss? Or all this time I've been misreading your 'saner' default as 'safer' default?
