Closed Bug 1528304 Opened 1 year ago Closed 1 year ago

-GS buffer overflow in arm64 breakpad

Categories

(Toolkit :: Crash Reporting, defect)

defect
Not set
normal

Tracking

()

RESOLVED FIXED
mozilla67
Tracking Status
firefox-esr60 --- wontfix
firefox65 --- wontfix
firefox66 + fixed
firefox67 + fixed

People

(Reporter: dmajor, Assigned: dmajor)

References

(Blocks 1 open bug)

Details

(Keywords: csectype-bounds, sec-moderate, Whiteboard: [post-critsmash-triage][adv-main66-])

Attachments

(1 file)

Bug 1518947 made it so that we collect arm64 registers for exception memory regions: https://searchfox.org/mozilla-central/rev/8e8ccec700159dc4efe061cfec2af10b21a8e62a/toolkit/crashreporter/breakpad-client/windows/common/minidump_callback.cc#215-222

However there are 32 registers on arm64 and the buffer is still sized at 16.

If Firefox crashes and there is no JIT on the stack, the -GS failure kills the process with a fastfail and no crash report is generated. (If there is JIT on the stack, other issues stand in the way. More in bug 1526276)

kExceptionAppMemoryRegions needs to be updated to at least 32: https://searchfox.org/mozilla-central/rev/8e8ccec700159dc4efe061cfec2af10b21a8e62a/toolkit/crashreporter/breakpad-client/windows/common/minidump_callback.h#48

Group: toolkit-core-security → core-security-release
Status: NEW → RESOLVED
Closed: 1 year ago
Resolution: --- → FIXED
Target Milestone: --- → mozilla67

David, want to request uplift to beta?

Flags: needinfo?(dmajor)

Comment on attachment 9044228 [details]
Update max memory recording regions.

Beta/Release Uplift Approval Request

Feature/Bug causing the regression

Bug 1518947

User impact if declined

Buffer overflow + missing crash reports

Is this code covered by automated tests?

Unknown

Has the fix been verified in Nightly?

No

Needs manual test from QE?

No

If yes, steps to reproduce

List of other uplifts needed

None

Risk to taking this patch

Low

Why is the change risky/not risky? (and alternatives if risky)

Buffer size fix

(Also, since I can't write a free-form answer to the automated tests question above, I'll write it here: this code would be covered by tests except that arm64 isn't running in CI yet.)

String changes made/needed

No

Flags: needinfo?(dmajor)
Attachment #9044228 - Flags: approval-mozilla-beta?

I checked crash-stats and we still haven't seen any crash reports after this landing, most likely because the other half of the puzzle (bug 1526276 comment 17) still stands in the way.

Comment on attachment 9044228 [details]
Update max memory recording regions.

First part of a potential fix for missing crash reports.
OK for uplift for beta 10.

Attachment #9044228 - Flags: approval-mozilla-beta? → approval-mozilla-beta+
Flags: qe-verify-
Whiteboard: [post-critsmash-triage]

Minusing for advisories since it is ARM64 only, which we aren't shipping yet.

Whiteboard: [post-critsmash-triage] → [post-critsmash-triage][adv-main66-]
Group: core-security-release
You need to log in before you can comment on or make changes to this bug.