1630607 - arm64: add debug-only high-bits-are-zero check in boxValue

Status: RESOLVED FIXED

(Core :: JavaScript Engine: JIT, enhancement, P1)

Description

[Benjamin Bouvier [:bbouvier] (inactive)]

This would have been quite useful to have to catch a bug in the new arm64 backend, and the x64 backend does it, so let's do it on arm64 too.

Comment 1

[Benjamin Bouvier [:bbouvier] (inactive)]

Attachment: Bug 1630607: arm64: check that upper high bits are zero'd when boxing a 32-bit value; r?jandem

Comment 2

[Benjamin Bouvier [:bbouvier] (inactive)]

https://treeherder.mozilla.org/#/jobs?repo=try&revision=548d72db825886034d7b8e5c1f77d81b97bfc9e4

Comment 3

[Pulsebot]

Pushed by [e-mail]:
https://hg.mozilla.org/integration/autoland/rev/60f939d5d3bc
arm64: check that upper high bits are zero'd when boxing a 32-bit value; r=jandem

Comment 4

[Cristina Coroiu [:ccoroiu]]

Backed out changeset 60f939d5d3bc (bug 1630607) for SM failure at workspace/breakpad-tools/libbreakpadinjector.so

Backout: https://hg.mozilla.org/integration/autoland/rev/aaa6e44260125781accde50de4db5aeefeb3011b

Failure push: https://treeherder.mozilla.org/#/jobs?repo=autoland&revision=60f939d5d3bc56097df7ee60bd58703b75969e53

Failure log: https://treeherder.mozilla.org/logviewer.html#/jobs?job_id=298056758&repo=autoland&lineNumber=18849

[task 2020-04-17T07:54:36.676Z] TEST-PASS | non262/String/split-01.js | (args: "--dll /builds/worker/workspace/breakpad-tools/libbreakpadinjector.so") [0.2 s]
[task 2020-04-17T07:54:36.682Z] ## non262/String/ropes.js: rc = -2, run time = 0.26164
[task 2020-04-17T07:54:36.683Z] Stress test of ropes
[task 2020-04-17T07:54:36.683Z] TEST-UNEXPECTED-FAIL | non262/String/ropes.js | (args: "--dll /builds/worker/workspace/breakpad-tools/libbreakpadinjector.so") [0.3 s]

Comment 5

[Benjamin Bouvier [:bbouvier] (inactive)]

Hiding until we understand what's going on. The failure of this check could mean that a JSValue's type is badly reinterpreted, which can be one step to arbitrary code execution.

Comment 6

[Benjamin Bouvier [:bbouvier] (inactive)]

Does anyone in the JIT team want to investigate the failure here (maybe showing a sec issue)? I'm running out of bandwidth here, and it might take a week or two before I can get back to this.

Comment 7

[Ted Campbell [:tcampbell]]

Attachment: Bug 1630607 - Fix LEffectiveAddress on ARM64

Comment 8

[Ted Campbell [:tcampbell]]

This is very likely a real sec issue. We emit a 64-bit LEA instruction on ARM64 when we should be using a 32-bit one. I'm doing a bit more digging to see if there are any mitigating factors that limit the severity, but so far it looks bad :(

Comment 9

[Ted Campbell [:tcampbell]]

I did some more digging and it looks like we can construct arbitrary type-confusion here. There is an ARM64-only issue.

Comment 10

[Ted Campbell [:tcampbell]]

I'm moving the fix to a new bug so it is easier to track. https://bugzilla.mozilla.org/show_bug.cgi?id=1631508

Comment 11

[Phabricator Automation]

Comment on attachment 9141714 [details]
Bug 1630607 - Fix LEffectiveAddress on ARM64

Revision D71567 was moved to bug 1631508. Setting attachment 9141714 [details] to obsolete.

Comment 12

[Benjamin Bouvier [:bbouvier] (inactive)]

Thanks Ted for looking at it!

Comment 13

[Release mgmt bot [:sylvestre / :calixte / :marco for bugbug]]

There's a r+ patch which didn't land and no activity in this bug for 2 weeks.
:bbouvier, could you have a look please?
For more information, please visit auto_nag documentation.

Comment 14

[Benjamin Bouvier [:bbouvier] (inactive)]

After discussing it with Jan, still waiting a bit on people updating so as to get the fix from bug 1631508 before landing this.

Comment 15

[Jan de Mooij [:jandem]]

I think we can land this now :)

Comment 16

[Benjamin Bouvier [:bbouvier] (inactive)]

Sounds good, will do a full, silent try-build first.

Comment 17

[Benjamin Bouvier [:bbouvier] (inactive)]

Try build returned green, after a few retriggers: https://treeherder.mozilla.org/#/jobs?repo=try&revision=b61dbdcb602f2503073d2cae34abc752139f6e2e

Will rebase and queue for landing now.

Comment 18

[Benjamin Bouvier [:bbouvier] (inactive)]

Attachment: Bug 1630607: arm64: check that upper high bits are zero'd when boxing a 32-bit value; r=jandem

Comment 19

[Benjamin Bouvier [:bbouvier] (inactive)]

https://hg.mozilla.org/integration/autoland/rev/e1f0deb14cf4e68c79ce3b3b42391e7f23318ed6

Comment 20

[Sebastian Hengst [:aryx] (needinfo on intermittent or backout)]

https://hg.mozilla.org/mozilla-central/rev/e1f0deb14cf4