[wasm][fuzzer] Optimize CheckMaxSteps with platform-specific code [v8/v8 : main]
Clemens Backes (Gerrit)
Clemens Backes added 1 comment![]( "Open in Gerrit")
PTAL.
This is both a performance and code size improvement (to make fuzzers faster) but it also fixes an issue with negative wraparound and with register pairs for 64-bit values on 32-bit platforms.
Related details
- Daniel Lehmann
Code-Owners
Code-Review
Review-Enforcement
| Inspect html for hidden footers to help with email filtering. To unsubscribe visit settings. |
SLSA Policy Verification Service (Gerrit)
SLSA Policy Verification Service voted SLSA-Policy-Verified+1![]( "Open in Gerrit")
| SLSA-Policy-Verified | +1 |
Related details
- Daniel Lehmann
Code-Owners
Code-Review
Review-Enforcement
| Inspect html for hidden footers to help with email filtering. To unsubscribe visit settings. |
Daniel Lehmann (Gerrit)
Daniel Lehmann voted Code-Review+1![]( "Open in Gerrit")
- Clemens Backes
Code-Owners
Code-Review
Review-Enforcement
| Inspect html for hidden footers to help with email filtering. To unsubscribe visit settings. |
Daniel Lehmann (Gerrit)
Daniel Lehmann voted Code-Review+0![]( "Open in Gerrit")
| Code-Review | +0 |
Related details
- Clemens Backes
Code-Owners
Code-Review
Review-Enforcement
| Inspect html for hidden footers to help with email filtering. To unsubscribe visit settings. |
Daniel Lehmann (Gerrit)
Daniel Lehmann added 1 comment![]( "Open in Gerrit")
Oops, mis-click, sorry for the noise.
Related details
- Clemens Backes
Code-Owners
Code-Review
Review-Enforcement
| Inspect html for hidden footers to help with email filtering. To unsubscribe visit settings. |
Daniel Lehmann (Gerrit)
Daniel Lehmann added 12 comments![]( "Open in Gerrit")
I mostly have understanding problems/questions, since I'm looking at our fuzzer max step mechanism for the first time.
3. Correct handling of 64-bit step counts (register pairs) on 32-bit
platforms (ia32, arm).I am not fully sure I understand: the step counter is a `int32_t*` pointer, i.e., can we even have 64-bit step counts? Or could we get rid of them if that makes things simpler? (This might be a stupid question / lacking context, since I never looked at our fuzzer step exhaustion mechanism so far.)
4. Robust prevention of underflow/wraparound exploits by saturating`exploits` made it sound like this has a security implication, but IIUC this would simply be a (at worst DoS) bug, where the fuzzer continues execution for longer than max steps (well, indefinitely until a timeout, i.e., the step mechanism would be useless/ignored in that case), right?
// If it underflowed, {Subs} will set the Carry (borrow) flag to 0.I like the explanation of the implementation details, but could you also add a high-level brief mention of the goal, namely that we want to saturate the step counter to `-1`.
Subs(max_steps.W(), max_steps.W(), reg.gp().W());Gemini tells me again that this line is buggy if `reg` contains a 64-bit step count, and one should use `max_steps.X()` instead? I'm still not quite sure (see also my high-level question on the commit message) whether we can have 64-bit step counts, so maybe this is WAI?
sbb(scratch, scratch);Same comment as below, I'd appreviate a very brief comment explaining/mentioning the saturation.
Register scratch = pinned.set(GetUnusedRegister(kGpReg, pinned)).gp();Since this needs 3 unused registers but those are really scarce on ia32, what happens or has it already happened in the past that we run out of registers?
if (auto* reg = std::get_if<LiftoffRegister>(&steps);nit: you could use an unconditional `std::get<LiftoffRegister>` here since the if above returns early otherwise, right?
CheckMaxSteps(decoder, static_cast<int32_t>(16 + __ num_locals()));Just to double check: `steps_done` is using an `int32_t` instead of `uint32_t` from `num_locals` to avoid silent unsigned integer overflow, right? Although in practice, that would have never been an issue, since `num_locals` is way below 2^31 due to our implementation limits, right?
sbbq(scratch, scratch);While clever, I didn't immediately understand the `sbbq` + `orl` saturating subtraction without some reviewing with Gemini. Can we add a comment here, explaining or at least mentioning this here (since readers are not going to dig up the commit message in the future to realize this saturation). Something something "If the previous subtraction underflowed (counter < steps), expand the carry flag to -1 and then mask with that, to cap the counter at -1."
subl(max_steps, std::get<LiftoffRegister>(steps).gp());Doesn't `subl` operate only on the 32-bit part of the register, i.e., would be wrong for 64-bit step counts?
Just to double check: This module contains an infinite loop or something that triggers this "fuel"/exhaustion fuzzer limits on execution?
Related details
- Clemens Backes
Code-Owners
Code-Review
No-Unresolved-Comments
Review-Enforcement
| Inspect html for hidden footers to help with email filtering. To unsubscribe visit settings. |
SLSA Policy Verification Service (Gerrit)
SLSA Policy Verification Service voted SLSA-Policy-Verified+1![]( "Open in Gerrit")
| SLSA-Policy-Verified | +1 |
Related details
- Clemens Backes
Code-Owners
Code-Review
No-Unresolved-Comments
Review-Enforcement
| Inspect html for hidden footers to help with email filtering. To unsubscribe visit settings. |
Clemens Backes (Gerrit)
Clemens Backes added 13 comments![]( "Open in Gerrit")
I mostly have understanding problems/questions, since I'm looking at our fuzzer max step mechanism for the first time.
Sorry, I should have pointed to https://crrev.com/c/7840281 somewhere for context. And I didn't choose Matthias as reviewer because he was out yesterday.
Sorry, it's a pretty large delta from PS3 to PS4 because we now need to pass the ValueKind to handle 64-bit values correctly on 64-bit platforms, but also handle register pairs on 32-bit platforms.
It's a fresh upload, if anything gets red I'll take a look tomorrow.
3. Correct handling of 64-bit step counts (register pairs) on 32-bit
platforms (ia32, arm).I am not fully sure I understand: the step counter is a `int32_t*` pointer, i.e., can we even have 64-bit step counts? Or could we get rid of them if that makes things simpler? (This might be a stupid question / lacking context, since I never looked at our fuzzer step exhaustion mechanism so far.)
We do pass a dynamic value for the number of steps to subtract. That value can e.g. come from the input to `memory.grow` and that can be an i64 value for memory64. That's not a great example as we can never grow by 2^32 pages, but it illustrates the problem.
4. Robust prevention of underflow/wraparound exploits by saturating`exploits` made it sound like this has a security implication, but IIUC this would simply be a (at worst DoS) bug, where the fuzzer continues execution for longer than max steps (well, indefinitely until a timeout, i.e., the step mechanism would be useless/ignored in that case), right?
Right.
// If it underflowed, {Subs} will set the Carry (borrow) flag to 0.I like the explanation of the implementation details, but could you also add a high-level brief mention of the goal, namely that we want to saturate the step counter to `-1`.
The goal is not to saturate at -1, but to prevent negative wraparound into the positive range. I added a comment.
Gemini tells me again that this line is buggy if `reg` contains a 64-bit step count, and one should use `max_steps.X()` instead? I'm still not quite sure (see also my high-level question on the commit message) whether we can have 64-bit step counts, so maybe this is WAI?
Hm, it has a point. We shouldn't just use `max_steps.X()`, but we should probably look at the upper half of the value as well, and stop immediately if any bit is set. Did that in the new patchset.
Same comment as below, I'd appreviate a very brief comment explaining/mentioning the saturation.
Done
Register scratch = pinned.set(GetUnusedRegister(kGpReg, pinned)).gp();Since this needs 3 unused registers but those are really scarce on ia32, what happens or has it already happened in the past that we run out of registers?
Hasn't happened yet. If it happens in fuzzing we will run into DCHECK failures and we will need to fix it.
if (auto* reg = std::get_if<LiftoffRegister>(&steps);nit: you could use an unconditional `std::get<LiftoffRegister>` here since the if above returns early otherwise, right?
Right, done.
CheckMaxSteps(decoder, static_cast<int32_t>(16 + __ num_locals()));Just to double check: `steps_done` is using an `int32_t` instead of `uint32_t` from `num_locals` to avoid silent unsigned integer overflow, right? Although in practice, that would have never been an issue, since `num_locals` is way below 2^31 due to our implementation limits, right?
Right, it shouldn't make a difference if we pass this as `uint32_t` or `int32_t`. There's a DCHECK below that ensures that it's always a relatively small integer anyway.
While clever, I didn't immediately understand the `sbbq` + `orl` saturating subtraction without some reviewing with Gemini. Can we add a comment here, explaining or at least mentioning this here (since readers are not going to dig up the commit message in the future to realize this saturation). Something something "If the previous subtraction underflowed (counter < steps), expand the carry flag to -1 and then mask with that, to cap the counter at -1."
Sure, totally makes sense. This is AI generated code of course, and AI often forgets to add some minimal comments. I'll try to remember to ask it more to add comments.
subl(max_steps, std::get<LiftoffRegister>(steps).gp());Doesn't `subl` operate only on the 32-bit part of the register, i.e., would be wrong for 64-bit step counts?
Right, this is the same problem as on arm64. I fixed it in the new patchset.
Just to double check: This module contains an infinite loop or something that triggers this "fuel"/exhaustion fuzzer limits on execution?
No, this triggered the DCHECK from the bug by executing a memory.grow on a 64-bit memory if I remember correctly.
Related details
- Daniel Lehmann
Code-Owners
Code-Review
No-Unresolved-Comments
Review-Enforcement
| Inspect html for hidden footers to help with email filtering. To unsubscribe visit settings. |