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

Status: RESOLVED FIXED

(Core :: JavaScript Engine, defect, P1)

Description

[Nicolas B. Pierron [:nbp]]

+++ 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.

Comment 1

[Nicolas B. Pierron [:nbp]]

Attachment: ARM: Zero the payload if the Value tag does not match the expected tag.

Comment 2

[Jan de Mooij [:jandem]]

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

Comment 3

[Nicolas B. Pierron [:nbp]]

(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.

Comment 4

[Jan de Mooij [:jandem]]

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

Comment 5

[Jan de Mooij [:jandem]]

(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.

Comment 6

[Nicolas B. Pierron [:nbp]]

try run with Android opt: https://treeherder.mozilla.org/#/jobs?repo=try&revision=d11bbe2a7e10ad445801050597b0e2213d954c9a

Comment 7

[Pulsebot]

Pushed by [e-mail]:
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

Comment 8

[Eliza Balazs [:ebalazs_]]

Backed out changeset 18c36451a875 (bug 1437510) for wpt failures in /fetch/api/request/request-idl.html

Push with failures: https://treeherder.mozilla.org/#/jobs?repo=mozilla-inbound&revision=18c36451a8750a46a51b531bc184f0ac290966c8&filter-resultStatus=testfailed&filter-resultStatus=busted&filter-resultStatus=exception&filter-resultStatus=runnable&filter-resultStatus=success&selectedJob=172694819

Failure log: https://treeherder.mozilla.org/#/jobs?repo=mozilla-inbound&revision=18c36451a8750a46a51b531bc184f0ac290966c8&filter-resultStatus=testfailed&filter-resultStatus=busted&filter-resultStatus=exception&filter-resultStatus=runnable&filter-resultStatus=success&selectedJob=172694819

Backout: https://treeherder.mozilla.org/#/jobs?repo=mozilla-inbound&revision=95268b8b70d9c60ec3a8994769fad53cfdbd36c1&filter-resultStatus=testfailed&filter-resultStatus=busted&filter-resultStatus=exception&filter-resultStatus=runnable&filter-resultStatus=success&filter-resultStatus=running&filter-resultStatus=pending

Comment 9

[Pulsebot]

Pushed by [e-mail]:
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

Comment 10

[Eliza Balazs [:ebalazs_]]

I relanded the patch. https://treeherder.mozilla.org/#/jobs?repo=mozilla-inbound&revision=30a4eaf2f075b32d9e60d04af9361169bf168eac&filter-resultStatus=testfailed&filter-resultStatus=busted&filter-resultStatus=exception&filter-resultStatus=runnable

Comment 11

[Andreea Pavel [:apavel]]

https://hg.mozilla.org/mozilla-central/rev/17916fce4252

Comment 12

[Nicolas B. Pierron [:nbp]]

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.

Comment 13

[Ryan VanderMeulen [:RyanVM]]

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.

Comment 14

[Ryan VanderMeulen [:RyanVM]]

https://hg.mozilla.org/releases/mozilla-beta/rev/9c8731ec8be6

Comment 15

[Cornel Ionce [:noni]]

(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-.