1678785 - AddressSanitizer: SEGV [@ vixl::Memory::Read]

Status: RESOLVED FIXED

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

Description

[Gary Kwong [:gkw] [:nth10sd] (NOT official MoCo now)]

This is test.wrapper, test.wasm is attached.

new WebAssembly.Instance(new WebAssembly.Module(read(scriptArgs[0], "binary"))).exports.g();

AddressSanitizer:DEADLYSIGNAL
=================================================================
==12641==ERROR: AddressSanitizer: SEGV on unknown address 0x7f3ffab5ffff (pc 0x55d9235f5166 bp 0x7fff7587aac0 sp 0x7fff7587a120 T0)
==12641==The signal is caused by a READ memory access.
    #0 0x55d9235f5165 in short vixl::Memory::Read<short, unsigned long>(unsigned long) /home/skygentoo/trees/mozilla-central/js/src/jit/arm64/vixl/Simulator-vixl.h:69:5
    #1 0x55d9235f5165 in short vixl::Simulator::Read<short>(unsigned long) /home/skygentoo/trees/mozilla-central/js/src/jit/arm64/vixl/Simulator-vixl.cpp:1042:12
    #2 0x55d9235f5165 in vixl::Simulator::LoadStoreHelper(vixl::Instruction const*, long, vixl::AddrMode) /home/skygentoo/trees/mozilla-central/js/src/jit/arm64/vixl/Simulator-vixl.cpp:1077:24
    #3 0x55d9234f1a99 in vixl::Decoder::VisitLoadStoreUnsignedOffset(vixl::Instruction const*) /home/skygentoo/trees/mozilla-central/js/src/jit/arm64/vixl/Decoder-vixl.cpp:876:1
    #4 0x55d9234f1a99 in vixl::Decoder::DecodeLoadStore(vixl::Instruction const*) /home/skygentoo/trees/mozilla-central/js/src/jit/arm64/vixl/Decoder-vixl.cpp:386:11
    #5 0x55d9235ee91f in vixl::Decoder::Decode(vixl::Instruction const*) /home/skygentoo/trees/mozilla-central/js/src/jit/arm64/vixl/Decoder-vixl.h:158:5
    #6 0x55d9235ee91f in vixl::Simulator::ExecuteInstruction() /home/skygentoo/trees/mozilla-central/js/src/jit/arm64/vixl/MozSimulator-vixl.cpp:224:13
    #7 0x55d9235ee91f in vixl::Simulator::Run() /home/skygentoo/trees/mozilla-central/js/src/jit/arm64/vixl/Simulator-vixl.cpp:70:5
    #8 0x55d9235e4af0 in vixl::Simulator::RunFrom(vixl::Instruction const*) /home/skygentoo/trees/mozilla-central/js/src/jit/arm64/vixl/Simulator-vixl.cpp:78:3
    #9 0x55d9235e4af0 in vixl::Simulator::call(unsigned char*, int, ...) /home/skygentoo/trees/mozilla-central/js/src/jit/arm64/vixl/MozSimulator-vixl.cpp:312:3
    #10 0x55d923eb121a in js::wasm::Instance::callExport(JSContext*, unsigned int, JS::CallArgs) /home/skygentoo/trees/mozilla-central/js/src/wasm/WasmInstance.cpp:2135:10
    #11 0x55d923f04d0d in WasmCall(JSContext*, unsigned int, JS::Value*) /home/skygentoo/trees/mozilla-central/js/src/wasm/WasmJS.cpp:1964:19
    #12 0x55d921beb695 in CallJSNative(JSContext*, bool (*)(JSContext*, unsigned int, JS::Value*), js::CallReason, JS::CallArgs const&) /home/skygentoo/trees/mozilla-central/js/src/vm/Interpreter.cpp:498:13
    #13 0x55d921beb695 in js::InternalCallOrConstruct(JSContext*, JS::CallArgs const&, js::MaybeConstruct, js::CallReason) /home/skygentoo/trees/mozilla-central/js/src/vm/Interpreter.cpp:590:12
    #14 0x55d921bd4f97 in InternalCall(JSContext*, js::AnyInvokeArgs const&, js::CallReason) /home/skygentoo/trees/mozilla-central/js/src/vm/Interpreter.cpp:643:10
    #15 0x55d921bd4f97 in js::CallFromStack(JSContext*, JS::CallArgs const&) /home/skygentoo/trees/mozilla-central/js/src/vm/Interpreter.cpp:647:10
    #16 0x55d921bd4f97 in Interpret(JSContext*, js::RunState&) /home/skygentoo/trees/mozilla-central/js/src/vm/Interpreter.cpp:3294:16
    #17 0x55d921bb7cea in js::RunScript(JSContext*, js::RunState&) /home/skygentoo/trees/mozilla-central/js/src/vm/Interpreter.cpp:468:13
    #18 0x55d921bf17f3 in js::ExecuteKernel(JSContext*, JS::Handle<JSScript*>, JS::Handle<JSObject*>, JS::Handle<JS::Value>, js::AbstractFramePtr, JS::MutableHandle<JS::Value>) /home/skygentoo/trees/mozilla-central/js/src/vm/Interpreter.cpp:835:13
    #19 0x55d921f0de1e in JS_ExecuteScript(JSContext*, JS::Handle<JSScript*>) /home/skygentoo/trees/mozilla-central/js/src/vm/CompilationAndEvaluation.cpp:457:10
    #20 0x55d921a549c5 in RunFile(JSContext*, char const*, _IO_FILE*, CompileUtf8, bool) /home/skygentoo/trees/mozilla-central/js/src/shell/js.cpp:974:10
    #21 0x55d921a531a4 in Process(JSContext*, char const*, bool, FileKind) /home/skygentoo/trees/mozilla-central/js/src/shell/js.cpp:1565:14
    #22 0x55d9219fac9c in ProcessArgs(JSContext*, js::cli::OptionParser*) /home/skygentoo/trees/mozilla-central/js/src/shell/js.cpp:10347:10
    #23 0x55d9219fac9c in Shell(JSContext*, js::cli::OptionParser*, char**) /home/skygentoo/trees/mozilla-central/js/src/shell/js.cpp:11046:10
    #24 0x55d9219ed396 in main /home/skygentoo/trees/mozilla-central/js/src/shell/js.cpp:11812:12
    #25 0x7f41fe924e8a in __libc_start_main (/lib64/libc.so.6+0x23e8a)
    #26 0x55d921926029 in _start (/home/skygentoo/shell-cache/js-64-asan-armsim64-linux-x86_64-fd1683e51ec5/js-64-asan-armsim64-linux-x86_64-fd1683e51ec5+0x2009029)

AddressSanitizer can not provide additional info.

Run with --fuzzing-safe --no-threads --no-baseline --no-ion test.wrapper test.wasm, compile with AR=ar sh ./configure --enable-simulator=arm64 --enable-address-sanitizer --disable-jemalloc --with-ccache --enable-gczeal --enable-debug-symbols --disable-tests, tested on m-c rev fd1683e51ec5.

Not sure if this is s-s or if it's ARM64 simulator-only, I'd leave it to Julian/Lars. Bisection result coming soon...

Comment 1

[Gary Kwong [:gkw] [:nth10sd] (NOT official MoCo now)]

Attachment: test.wasm

Comment 2

[Gary Kwong [:gkw] [:nth10sd] (NOT official MoCo now)]

Attachment: Debug stack

Debug shell compiled with:

AR=ar sh ./configure --enable-simulator=arm64 --enable-debug --with-ccache --enable-gczeal --enable-debug-symbols --disable-tests

Run with: --fuzzing-safe --no-threads --no-baseline --no-ion test.wrapper test.wasm

Comment 3

[Gary Kwong [:gkw] [:nth10sd] (NOT official MoCo now)]

Attachment: Opt stack

(gdb) x/i $pc
=> 0x55555774e873 <vixl::Simulator::LoadStoreHelper(vixl::Instruction const*, long, vixl::AddrMode)+5779>:	movzwl (%rbx),%eax
(gdb) x/b $rbx
0x7ffe75eeefff:	Cannot access memory at address 0x7ffe75eeefff
(gdb) x/b $eax
0x0:	Cannot access memory at address 0x0

Compile with:

AR=ar sh ./configure --enable-simulator=arm64 --with-ccache --enable-gczeal --enable-debug-symbols --disable-tests

Run with:

--fuzzing-safe --no-threads --no-baseline --no-ion test.wrapper test.wasm

Comment 4

[Gary Kwong [:gkw] [:nth10sd] (NOT official MoCo now)]

Due to skipped revisions, the first bad revision could be any of:
changeset:   https://hg.mozilla.org/mozilla-central/rev/e9788db7f9fd
user:        Julian Seward
date:        Thu Nov 19 18:50:12 2020 +0000
summary:     Bug 1677452 - Update Cranelift to firefox85 / dcc52ba3f69d3de7cdbd787b936825d9c61e3c27 and wasmparser to 0.67: Part 1 - hash and API changes.  r=lth.

changeset:   https://hg.mozilla.org/mozilla-central/rev/970a61b06242
user:        Julian Seward
date:        Thu Nov 19 18:50:58 2020 +0000
summary:     Bug 1677452 - Update Cranelift to firefox85 / dcc52ba3f69d3de7cdbd787b936825d9c61e3c27 and wasmparser to 0.67: Part 2 - results of "mach vendor rust". r=lth.

Lars/Julian, any thoughts?

Comment 5

[Christian Holler (:decoder)]

This doesn't reproduce on actual hardware for me. However, I filed bug 1678582 yesterday which is an invalid read related to Cranelift, so maybe this is related.

Comment 6

[Lars T Hansen [:lth]]

Since the evidence is simulator-only for now we'll leave it at P2, but we should look into this soon.

Comment 7

[Lars T Hansen [:lth]]

I can repro locally with --wasm-compiler=cranelift; with --wasm-compiler=baseline I see an error:

test.wrapper line 1 > WebAssembly.Module:67:1 RuntimeError: index out of bounds
Stack:
  @test.wrapper line 1 > WebAssembly.Module:wasm-function[2]:0x43
  @test.wrapper:1:89

Comment 8

[Lars T Hansen [:lth]]

Looks like a real (and fairly bad) cranelift bug.

Test program follows. Note only the last function is exported and called; the others are unreferenced, so I've omitted their bodies.

(module
  (type (;0;) (func))
  (type (;1;) (func (result i32)))
  (type (;2;) (func (result i64)))
  (func (;0;) (type 2) (result i64) ...)
  (func (;1;) (type 1) (result i32) ...)
  (func (;2;) (type 0)
    i32.const -4098
    i32.load16_s offset=1
    drop)
  (memory (;0;) 1)
  (export "g" (func 2)))

Baseline code for the body:

0x15c6826a11d4  12820000  mov     w0, #0xffffefff              ;; -4098 as 32-bit, ie 0x00000000ffffefff in x0
0x15c6826a11d8  78e06aa0  ldrsh   w0, [x21, x0]                ;; load from memory

Cranelift code:

0x1ee4f34c1160  92820000  mov     x0, #0xffffffffffffefff  ;; -4098 as 64-bit value
0x1ee4f34c1164  8b150000  add     x0, x0, x21              ;; load from
0x1ee4f34c1168  79800000  ldrsh   x0, [x0]                 ;;   memory

The difference seems to be that cranelift sign extends the offset (the wasm program's address) to 64 bits while baseline zero extends it. Baseline is right here: wasm i32 values are to be treated as unsigned when used as memory addresses.

The baseline instruction 78e06aa0 (LDRSH) uses the raw value of x0 for the address computation, from what I can tell from the encoding.

Normally there will be stuff in memory below the heap. By manipulating the negative address (admittedly these need to be constants in the code), those contents could be read. It's probably not predictable what's there and it would be easy to crash, but there's an attack vector.

Comment 10

[Lars T Hansen [:lth]]

Something questionable in lower_address, presumably. The case of a negative constant operand is tricky.

Comment 11

[Julian Seward [:jseward]]

I verified that the bug is in the CLIF->aarch64 translation phase and not in the
wasm->CLIF translation. The CLIF looks correct, in particular the definitions
for v3 and v6:

                                    block0(v0: i64, v1: i64, v2: i64):
    @0040                               v3 = iconst.i32 -4098
    @0043                               v6 = uextend.i64 v3
    @0043                               v7 = get_pinned_reg.i64 
    @0043                               v4 = iadd v7, v6
    @0043                               v5 = sload16.i32 v4+1
    @0047                               jump block1

but that somehow gets transformed into

    Block 0:
      Inst 0:   mov %v0J, x23
      Inst 1:   movn %v8J, #4096
      Inst 2:   add %v8J, %v8J, x21/pinned_reg
      Inst 3:   ldrsh %v7J, [%v8J]
      Inst 4:   b label1

which, as Lars observes, appears to have sign- rather than zero extended
the -4098.

Comment 12

[Lars T Hansen [:lth]]

My bad, meant to assign to julian.

Comment 13

[Julian Seward [:jseward]]

This will be fixed by revendoring CL to 3bbadf8a2152984cf3d327d0c5a2677f37a93801
from https://github.com/mozilla-spidermonkey/wasmtime, branch firefox85.

This pulls in https://github.com/bytecodealliance/wasmtime/pull/2478/commits,
which fixes a bug in the AArch64 instruction selector, that was causing incorrect
load/store address generation in an obscure case.

Comment 14

[Julian Seward [:jseward]]

Attachment: Bug 1678785 - AddressSanitizer: SEGV [@ vixl::Memory::Read]. r=lth.

Comment 15

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

https://hg.mozilla.org/integration/autoland/rev/3d1ed513b353c6ec003383fa45f8c6b8a5957028
https://hg.mozilla.org/mozilla-central/rev/3d1ed513b353

Comment 16

[Julien Cristau [:jcristau]]

What's the status of this on branches?

Comment 17

[Lars T Hansen [:lth]]

Cranelift is nightly-only.

Comment 18

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

As part of a security bug pattern analysis, we are requesting your help with a high level analysis of this bug. It is our hope to develop static analysis (or potentially runtime/dynamic analysis) in the future to identify classes of bugs.

Please visit this google form to reply.