Closed Bug 1437510 Opened 2 years ago Closed 2 years ago

Spectre mitigations for Value type checks - ARM part (32 bit)

Categories

(Core :: JavaScript Engine, defect, P1)

defect

Tracking

()

RESOLVED FIXED
mozilla61
Tracking Status
firefox60 --- fixed
firefox61 --- fixed

People

(Reporter: nbp, Assigned: nbp)

References

(Blocks 1 open bug)

Details

Attachments

(1 file)

+++ This bug was initially created as a clone of Bug #1433111 +++

This bug is about implementing Spectre mitigation for JSValue Type guards unboxing on ARM 32 bits.
Assignee: nobody → nicolas.b.pierron
Status: NEW → ASSIGNED
(In reply to Jan de Mooij [:jandem] from comment #2)
> Don't we need something like this on ARM too:
> 
> https://searchfox.org/mozilla-central/rev/
> d0413b711da4dac72b237d0895daba2537f1514c/js/src/jit/x86/Lowering-x86.cpp#122-
> 125

No, and I think this is no longer needed on x86 as well.

I guess I kept it in the previous patch and forgot to reconsider it after Bug 1433111 comment 7.  The problem was that the dest-ination register could alias the other registers before. With this pattern, 

    push(0)
    cmp type, …
    cmove  payload, dest
    cmovne (esp, 0), dest
    add 4, esp

All the registers are read before setting the dest-ination register to its final value.

I will double check x86 after reverting the lowering back to its original code.
Flags: needinfo?(nicolas.b.pierron)
Comment on attachment 8965348 [details] [diff] [review]
ARM: Zero the payload if the Value tag does not match the expected tag.

Review of attachment 8965348 [details] [diff] [review]:
-----------------------------------------------------------------

Nice. ARM makes this a lot simpler than x86.

::: js/src/jit/arm/CodeGenerator-arm.cpp
@@ +1451,5 @@
>  #endif
>      }
> +
> +    // Note: If spectreValueMasking is disabled, then this instruction will
> +    // default to a no-op as long as the lowering allocate the same register for

Nit: allocates
Attachment #8965348 - Flags: review?(jdemooij) → review+
(In reply to Nicolas B. Pierron [:nbp] {backlog: ~36} from comment #3)
> No, and I think this is no longer needed on x86 as well.

Does it still avoid the stack push on x86? In that case I think it's still nice to keep it.
Pushed by npierron@mozilla.com:
https://hg.mozilla.org/integration/mozilla-inbound/rev/18c36451a875
ARM: Zero the payload if the Value tag does not match the expected tag. r=jandem
Pushed by ebalazs@mozilla.com:
https://hg.mozilla.org/integration/mozilla-inbound/rev/17916fce4252
ARM: Zero the payload if the Value tag does not match the expected tag. r=jandem
Flags: needinfo?(nicolas.b.pierron)
https://hg.mozilla.org/mozilla-central/rev/17916fce4252
Status: ASSIGNED → RESOLVED
Closed: 2 years ago
Resolution: --- → FIXED
Target Milestone: --- → mozilla61
Comment on attachment 8965348 [details] [diff] [review]
ARM: Zero the payload if the Value tag does not match the expected tag.

Approval Request Comment
[Feature/Bug causing the regression]: N/A
[User impact if declined]: We want to uplift Spectre fixes in 61 to 60.
[Is this code covered by automated tests?]: Yes.
[Has the fix been verified in Nightly?]: Yes.
[Needs manual test from QE? If yes, steps to reproduce]: No.
[List of other uplifts needed for the feature/fix]: Bug 1433111 (should already be uplifted)
[Is the change risky?]: No.
[Why is the change risky/not risky?]: Already done in x86, mimic on ARM.
[String changes made/needed]: None.
Attachment #8965348 - Flags: approval-mozilla-beta?
Comment on attachment 8965348 [details] [diff] [review]
ARM: Zero the payload if the Value tag does not match the expected tag.

Add more Spectre mitigations for ARM. Approved for 60.0b13.
Attachment #8965348 - Flags: approval-mozilla-beta? → approval-mozilla-beta+
(In reply to Nicolas B. Pierron [:nbp] {backlog: ~36} from comment #12)
> [Is this code covered by automated tests?]: Yes.
> [Needs manual test from QE? If yes, steps to reproduce]: No.

Since this is already covered by automated tests and does not require manual testing, marking as qe-verify-.
Flags: qe-verify-
You need to log in before you can comment on or make changes to this bug.