Closed Bug 1630607 Opened 11 months ago Closed 9 months ago

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


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




Tracking Status
firefox-esr68 --- wontfix
firefox76 --- wontfix
firefox77 --- wontfix
firefox78 --- fixed


(Reporter: bbouvier, Assigned: bbouvier)



(Keywords: sec-other, Whiteboard: [keep hidden while bug 1631508 is][post-critsmash-triage][adv-main78-])


(1 file, 2 obsolete files)

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.

Priority: -- → P1
Pushed by
arm64: check that upper high bits are zero'd when boxing a 32-bit value; r=jandem

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


Failure push:

Failure log:

[task 2020-04-17T07:54:36.676Z] TEST-PASS | non262/String/split-01.js | (args: "--dll /builds/worker/workspace/breakpad-tools/") [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/") [0.3 s]

Flags: needinfo?(bbouvier)
Group: core-security, javascript-core-security

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.

Group: core-security

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.

Flags: needinfo?(jdemooij)

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

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

Hardware: Unspecified → ARM64

I'm moving the fix to a new bug so it is easier to track.

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

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

Attachment #9141714 - Attachment is obsolete: true
Flags: needinfo?(jdemooij)

Thanks Ted for looking at it!

Flags: needinfo?(bbouvier)
Keywords: sec-other
See Also: → 1631508
Whiteboard: [keep hidden while bug 1631508 is]

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.

Flags: needinfo?(bbouvier)

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

Flags: needinfo?(bbouvier)
Severity: -- → N/A

I think we can land this now :)

Flags: needinfo?(bbouvier)

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

Try build returned green, after a few retriggers:

Will rebase and queue for landing now.

Flags: needinfo?(bbouvier)
Attachment #9151662 - Attachment is obsolete: true
Group: javascript-core-security → core-security-release
Closed: 9 months ago
Resolution: --- → FIXED
Target Milestone: --- → mozilla78
Flags: qe-verify-
Whiteboard: [keep hidden while bug 1631508 is] → [keep hidden while bug 1631508 is][post-critsmash-triage]
Whiteboard: [keep hidden while bug 1631508 is][post-critsmash-triage] → [keep hidden while bug 1631508 is][post-critsmash-triage][adv-main78-]
Group: core-security-release
You need to log in before you can comment on or make changes to this bug.