1693476 - jit-test/tests/wasm/exceptions/throw-to-js.js: Generated code accesses below stack pointer

Status: RESOLVED FIXED

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

Description

[Julian Seward [:jseward]]

This is probably harmless from a sec- perspective; hiding nevertheless.

On aarch64-linux, this

 ./dist/bin/js \
  -f /home/sewardj/MOZ/IONA64/MUV/js/src/jit-test/lib/prologue.js \
  --wasm-compiler=baseline --wasm-exceptions \
  -e 'const platform='"'"'linux'"'"'' \
  -e 'const libdir='"'"'/home/sewardj/MOZ/IONA64/MUV/js/src/jit-test/lib/'"'"'' \
  -e 'const scriptdir='"'"'/home/sewardj/MOZ/IONA64/MUV/js/src/jit-test/tests/wasm/exceptions/'"'"'' \
  -f /home/sewardj/MOZ/IONA64/MUV/js/src/jit-test/lib/wasm.js \
  -e 'if ((!wasmExceptionsEnabled())) quit(59)' \
  --module-load-path /home/sewardj/MOZ/IONA64/MUV/js/src/jit-test/modules/ \
   -f /home/sewardj/MOZ/IONA64/MUV/js/src/jit-test/tests/wasm/exceptions/throw-to-js.js

produces

==470552== Invalid read of size 8
==470552==    at 0x16511B59893C: ???
==470552==  Address 0x1ffeffe1b0 is on thread 1's stack
==470552==  64 bytes below stack pointer

   0x000016511b598900:  mov     x16, sp
   0x000016511b598904:  and     sp, x16, #0xfffffffffffffff0
   0x000016511b598908:  sub     sp, sp, #0x30
   0x000016511b59890c:  mov     x0, sp
   0x000016511b598910:  ldr     x16, 0x16511b598970
   0x000016511b598914:  blr     x16
   0x000016511b598918:  ldr     w8, [x0, #24]
   0x000016511b59891c:  cmp     w8, #0x6
   0x000016511b598920:  b.eq    0x16511b598930  // b.none
   0x000016511b598924:  cmp     w8, #0x5
   0x000016511b598928:  b.eq    0x16511b598950  // b.none
   0x000016511b59892c:  brk     #0x0
   0x000016511b598930:  ldr     x29, [x0]
   0x000016511b598934:  ldr     x16, [x0, #8]
   0x000016511b598938:  mov     sp, x16
=> 0x000016511b59893c:  ldr     x8, [x0, #32]  <============== here
   0x000016511b598940:  eor     x9, x8, #0xfffe000000000000
   0x000016511b598944:  ldr     x8, [x0, #16]
   0x000016511b598948:  mov     x10, x9
   0x000016511b59894c:  br      x8
   0x000016511b598950:  ldr     x29, [x0]
   0x000016511b598954:  ldr     x8, [x0, #8]
   0x000016511b598958:  mov     sp, x8
   0x000016511b59895c:  ldr     x30, [x8]
   0x000016511b598960:  add     sp, sp, #0x8
   0x000016511b598964:  ret
   0x000016511b598968:  b       0x16511b598978
   0x000016511b59896c:  .inst   0xffff0003 ; undefined
   0x000016511b598970:  .inst   0x00e51c90 ; undefined
   0x000016511b598974:  udf     #0
   0x000016511b598978:  hlt     #0xbaad
   0x000016511b59897c:  hlt     #0xbaad
   0x000016511b598980:  udf     #0

and a bit of poking about with IONFLAGS=codegen suggests that
the fragment above is part of

[Codegen] # Emitting wasm exit stubs

Comment 1

[Lars T Hansen [:lth]]

Adding asumu because this is in an exception test case, but stressing we do not know if the problem is introduced by the exception handling.

Comment 2

[Lars T Hansen [:lth]]

Sorry, should have ni?d. Asumu, any thoughts?

Comment 3

[Asumu Takikawa]

Hi there, the generated code does look suspiciously like it might've come from the throw stub code changes from exceptions. I just got access to an ARM64 machine that I can test this on, so I will investigate this more and report back on this bug.

Comment 4

[Julian Seward [:jseward]]

The problem originates in (code created by) GenerateThrowStub in WasmStubs.cpp. It appears to pull a "previous frame's stack pointer" (or some such) value from the stack, hence "unprotecting" an area of stack which subsequent instructions continue to read from. Specifically, here is an interleaving of the relevant bits of code generator and generated code:

  masm.branch32(Assembler::Equal, scratch,
                Imm32(jit::ResumeFromException::RESUME_WASM_CATCH),
                &resumeCatch);
  masm.branch32(Assembler::Equal, scratch,
                Imm32(jit::ResumeFromException::RESUME_WASM), &leaveWasm);

  masm.breakpoint();

[Codegen] [1] 7100191f        cmp     w8, #0x6 (6)
[Codegen] [1] 54000000        b.eq    -> 1020f
[Codegen] [1] 7100151f        cmp     w8, #0x5 (5)
[Codegen] [1] 54000000        b.eq    -> 1021f
[Codegen] [1] d4200000        brk     #0x0

  // The case where a Wasm catch handler was found while unwinding the stack.
  masm.bind(&resumeCatch);

Codegen] [1]           1020:

  masm.loadPtr(Address(ReturnReg, offsetof(ResumeFromException, framePointer)),
               FramePointer);

[Codegen] [1] f940001d        ldr     x29, [x0]

// **** This next line is the problem.  After this point, we make two further
// reads based on ReturnReg but those are now below the just-reloaded stackPointer.

  masm.loadStackPtr(
      Address(ReturnReg, offsetof(ResumeFromException, stackPointer)));

[Codegen] [1] f9400410        ldr     x16, [x0, #8]
[Codegen] [1] 9100021f        mov     sp, x16

  // When there is a catch handler, HandleThrow passes it the Value needed for
  // the handler's argument as well.
#ifdef JS_64BIT
  ValueOperand val(scratch);
#else
  ValueOperand val(scratch, scratch2);
#endif
  masm.loadValue(Address(ReturnReg, offsetof(ResumeFromException, exception)),
                 val);

[Codegen] [1] f9401008        ldr     x8, [x0, #32]

  Register obj = masm.extractObject(val, scratch2);

[Codegen] [1] d24f3909        eor     x9, x8, #0xfffe000000000000

  masm.loadPtr(Address(ReturnReg, offsetof(ResumeFromException, target)),
               scratch);

[Codegen] [1] f9400808        ldr     x8, [x0, #16]

  masm.movePtr(obj, WasmExceptionReg);
  masm.jump(scratch);

[Codegen] [1] aa0903ea        mov     x10, x9
[Codegen] [1] d61f0100        br      x8

Comment 5

[Julian Seward [:jseward]]

Attachment: proposed-fix-1693476.diff

Here's a possible fix; at least it removes the below-SP access in the test case. I haven't tested it much further. Asumu, does this look correct/sufficient to you?

Comment 6

[Asumu Takikawa]

Thanks Julian, I was able to reproduce this and your fix looks good for arm64 but I think it's slightly broken for x64. The WasmExceptionReg in masm.movePtr(obj, WasmExceptionReg) happens to be ABINonArgReg0/ReturnReg on x64, so the SP reload has to go right before that line instead of right after.

With that change, it passes exception tests (and other jit-tests) for me on both x64 and arm64, and I don't see the below-SP error in valgrind either.

Comment 7

[Julian Seward [:jseward]]

Asumu, thanks for checking this out. Are you happy to wrap this up, or do you prefer I do it? (Is fine by me either way).

Comment 8

[Asumu Takikawa]

I'm happy to do it, will attach a patch via phabricator later today. Thanks again.

Comment 9

[Asumu Takikawa]

Attachment: Bug 1693476 - fix SP reload in the Wasm throw stub

Fixes an issue in which the Wasm throw stub could generate
bad accesses under the SP when a catch handler was found.

Comment 10

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

Daniel, is this patch accepted to land? https://wiki.mozilla.org/Security_Severity_Ratings/Client#Alternate_Keywords suggests separate bugs get filed for issues found in sec-audit ones.

Comment 11

[Daniel Veditz [:dveditz]]

This is fine to land

Comment 12

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

fix SP reload in the Wasm throw stub r=rhunt
https://hg.mozilla.org/integration/autoland/rev/507a60ec1f75fcea6b01f5dbe87e4fb818b8803d

Comment 13

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

https://hg.mozilla.org/mozilla-central/rev/507a60ec1f75