1774449 - ARM64 trap exit stub may access misaligned SP

Status: RESOLVED FIXED

(Core :: JavaScript: WebAssembly, defect, P2)

Description

[Markus Stange [:mstange]]

Crash report: https://crash-stats.mozilla.org/report/index/1dff9208-40d8-4914-83ff-7d6830220615

Reason: EXC_BAD_ACCESS / 0x00000103

Crashing address: 0x000000016f49fad8

Top 5 frames of crashing thread:

0 None @0x000020c60254b2f8 
1 None @0x000020c602542cb4 
2 None @0x000020c602542cb4 
3 None @0x000020c602520720 
4 None @0x000020c6025216b0 

I encountered this crash only once and can't reproduce it, but it looks bad.

The crash happened in a worker, which is probably running this asm.js script: https://github.com/firefox-devtools/profiler/blob/main/res/zee-worker.js

Steps to not reproduce:

1. Download the gz from https://drive.google.com/file/d/1LKDw2yRzzjm8coxp_BXK_dczFwPFuzjD/view?usp=sharing . (originally from bug 1774027 comment 4)
2. Open a tab with https://profiler.firefox.com/
3. Open the Downloads dropdown, and drag the downloaded file from the dropdown into the profiler.

Now the tab crashed.

Comment 1

[Ryan Hunt [:rhunt] (on leave until early May)]

I was able to reproduce this once too. Investigating.

Comment 2

[Ryan Hunt [:rhunt] (on leave until early May)]

Bottom frame:

    0x2150bc3399bc: 0xd45755a0   hlt    #0xbaad
    0x2150bc3399c0: 0xd10043ff   sub    sp, sp, #0x10             ; =0x10 
    0x2150bc3399c4: 0xd280a730   mov    x16, #0x539
->  0x2150bc3399c8: 0xf90003f0   str    x16, [sp]
    0x2150bc3399cc: 0xa9be6ffa   stp    x26, x27, [sp, #-0x20]!
    0x2150bc3399d0: 0xa90177fc   stp    x28, x29, [sp, #0x10]

Previous frame:

    0x2150bc3302fc: 0xf2400f9f   tst    x28, #0xf
    0x2150bc330300: 0x54000040   b.eq   0x2150bc330308
    0x2150bc330304: 0xd4200000   brk    #0
    0x2150bc330308: 0x9100039f   mov    sp, x28
    0x2150bc33030c: 0x97ff9189   bl     0x2150bc314930
->  0x2150bc330310: 0xb9400ae0   ldr    w0, [x23, #0x8]
    0x2150bc330314: 0xb9436381   ldr    w1, [x28, #0x360]
    0x2150bc330318: 0x6b00003f   cmp    w1, w0

The bottom frame is the trap exit created by GenerateTrapExit. Specifically we're trapping on:

WasmPush(masm, ImmWord(TrapExitDummyValue));

The trap is EXC_BAD_ACCESS (code=259, address=0x16cb3b748), the address is the SP. The SP is not aligned correctly for ARM64 (16 byte), causing the trap. The memory region pointed to by SP is read-writable, so that's another confirmation of the issue.

The trap exit stub notes that SP is not guaranteed to be stack-aligned and performs a dynamic alignment. The issue is that it's done after we push to the stack, which can cause this exception on ARM64. I'll look into how to resolve this.

If this is the issue that mstange is running into, I don't believe this is security sensitive as this should reliably crash when it happens. However, from looking at the crash dump linked to I see the address is 0x103 which doesn't look like a normal SP. So I'm not 100% confident yet of what's going on.

Comment 3

[Markus Stange [:mstange]]

(In reply to Ryan Hunt [:rhunt] from comment #2)

from looking at the crash dump linked to I see the address is 0x103

No the address is 0x000000016f49fad8, which does look like an 8-byte aligned stack address. The 0x103 is just the EXC_BAD_ACCESS code.

Comment 4

[Ryan Hunt [:rhunt] (on leave until early May)]

(In reply to Markus Stange [:mstange] from comment #3)

(In reply to Ryan Hunt [:rhunt] from comment #2)

from looking at the crash dump linked to I see the address is 0x103

No the address is 0x000000016f49fad8, which does look like an 8-byte aligned stack address. The 0x103 is just the EXC_BAD_ACCESS code.

Ah, thank you, that makes more sense. I'm likely seeing the same issue then.

Comment 5

[Ryan Hunt [:rhunt] (on leave until early May)]

cc'ing Julian who has some experience with dealing with ARM64 stack issues.

Fixing this appears to be tricky due to the constraints on this stub function.

1. The trap stub may be entered with a misaligned stack pointer
2. The trap stub must preserve (essentially) all registers as it may return back into the wasm function it was redirected from.
   • wasm functions assume that no registers are clobbered when executing an instruction that may potentially trap and return
3. (implication of point 2) The incoming (potentially misaligned) stack pointer must be preserved by this function
4. ARM64 cannot spill any registers using SP until it is aligned

Comment 6

[Jan de Mooij [:jandem]]

Attachment: Bug 1774449 - Always align SP to 16 bytes in Wasm Ion codegen for ARM64. r?rhunt!

This fixes a regression from bug 1770366 part 4: we used to align the local size to
JitStackAlignment even for Wasm, but that patch changed this to pointer-alignment.
(Wasm code uses WasmStackAlignment only if needsStaticStackAlignment() is true.)

On ARM64 this is wrong because we need the 16-byte alignment. It usually still works
because of the pseudo-stack-pointer, but falls apart if we have to call a trap
stub because it uses the actual stack pointer.

Comment 7

[Jan de Mooij [:jandem]]

This is from bug 1770366, sorry about that :/

Comment 8

[BugBot [:suhaib / :marco/ :calixte]]

Set release status flags based on info from the regressing bug 1770366

Comment 9

[Markus Stange [:mstange]]

(In reply to Ryan Hunt [:rhunt] from comment #2)

If this is the issue that mstange is running into, I don't believe this is security sensitive as this should reliably crash when it happens.

That sounds right to me. Should somebody open the bug then?

Comment 10

[Sebastian Hengst [:aryx] (needinfo me if it's about an intermittent or backout)]

Always align SP to 16 bytes in Wasm Ion codegen for ARM64. r=rhunt
https://hg.mozilla.org/integration/autoland/rev/9716d1d26289cc4d4bd0d9f64f0e8e6241cf7fed
https://hg.mozilla.org/mozilla-central/rev/9716d1d26289

Comment 12

[BugBot [:suhaib / :marco/ :calixte]]

Set release status flags based on info from the regressing bug 1770366