1843631 - [x86-32] Incorrect trap site metadata for Wasm-GC access

Status: RESOLVED DUPLICATE of bug 1846474

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

Description

[P1umer]

Attachment: poc1.js

User Agent: Mozilla/5.0 (Macintosh; Intel Mac OS X 10_15_7) AppleWebKit/537.36 (KHTML, like Gecko) Chrome/114.0.0.0 Safari/537.36

Steps to reproduce:

The vulnerability exists in the WASM engine. The configuration for building is as follows:

ac_add_options --enable-project=js
ac_add_options --enable-optimize
ac_add_options --disable-debug
ac_add_options --enable-debug-symbols
ac_add_options --disable-tests
ac_add_options --disable-jemalloc
ac_add_options --enable-address-sanitizer
ac_add_options --enable-address-sanitizer
ac_add_options --target=i686-pc-linux-gnu
ac_add_options --host=x86_64-pc-linux-gnu

Commit: a06c18043ff6bd5b70021a83b4fd0bcc3fc9b968

Flags:
--wasm-extended-const
--wasm-function-references
--wasm-gc
--wasm-memory-control
--wasm-compiler=ion

This is the full crash log:

AddressSanitizer:DEADLYSIGNAL
=================================================================
==1641316==ERROR: AddressSanitizer: SEGV on unknown address 0x00000010 (pc 0x4b93b09d bp 0xffd28608 sp 0xffd28604 T0)
==1641316==The signal is caused by a WRITE memory access.
==1641316==Hint: address points to the zero page.
    #0 0x4b93b09d  (<unknown module>)
    #1 0x4b93b189  (<unknown module>)
    #2 0x5b35c502 in js::wasm::Instance::callExport(JSContext*, unsigned int, JS::CallArgs, js::wasm::CoercionLevel) /home/p1umer/Git/gecko-dev/js/src/wasm/WasmInstance.cpp:2620:10
    #3 0x5b3d1280 in WasmCall(JSContext*, unsigned int, JS::Value*) /home/p1umer/Git/gecko-dev/js/src/wasm/WasmJS.cpp:2080:19
    #4 0x5811f024 in CallJSNative(JSContext*, bool (*)(JSContext*, unsigned int, JS::Value*), js::CallReason, JS::CallArgs const&) /home/p1umer/Git/gecko-dev/js/src/vm/Interpreter.cpp:486:13
    #5 0x5811f024 in js::InternalCallOrConstruct(JSContext*, JS::CallArgs const&, js::MaybeConstruct, js::CallReason) /home/p1umer/Git/gecko-dev/js/src/vm/Interpreter.cpp:580:12
    #6 0x5814a681 in InternalCall(JSContext*, js::AnyInvokeArgs const&, js::CallReason) /home/p1umer/Git/gecko-dev/js/src/vm/Interpreter.cpp:647:10
    #7 0x5814a681 in js::CallFromStack(JSContext*, JS::CallArgs const&, js::CallReason) /home/p1umer/Git/gecko-dev/js/src/vm/Interpreter.cpp:652:10
    #8 0x5814a681 in js::Interpret(JSContext*, js::RunState&) /home/p1umer/Git/gecko-dev/js/src/vm/Interpreter.cpp:3395:16
    #9 0x5811d92d in MaybeEnterInterpreterTrampoline(JSContext*, js::RunState&) /home/p1umer/Git/gecko-dev/js/src/vm/Interpreter.cpp:400:10
    #10 0x5811d92d in js::RunScript(JSContext*, js::RunState&) /home/p1umer/Git/gecko-dev/js/src/vm/Interpreter.cpp:458:13
    #11 0x58126a12 in js::ExecuteKernel(JSContext*, JS::Handle<JSScript*>, JS::Handle<JSObject*>, js::AbstractFramePtr, JS::MutableHandle<JS::Value>) /home/p1umer/Git/gecko-dev/js/src/vm/Interpreter.cpp:845:13
    #12 0x58126a12 in js::Execute(JSContext*, JS::Handle<JSScript*>, JS::Handle<JSObject*>, JS::MutableHandle<JS::Value>) /home/p1umer/Git/gecko-dev/js/src/vm/Interpreter.cpp:877:10
    #13 0x5842aa94 in ExecuteScript(JSContext*, JS::Handle<JSObject*>, JS::Handle<JSScript*>, JS::MutableHandle<JS::Value>) /home/p1umer/Git/gecko-dev/js/src/vm/CompilationAndEvaluation.cpp:493:10
    #14 0x5842b2ec in JS_ExecuteScript(JSContext*, JS::Handle<JSScript*>) /home/p1umer/Git/gecko-dev/js/src/vm/CompilationAndEvaluation.cpp:517:10
    #15 0x57f82ad4 in RunFile(JSContext*, char const*, _IO_FILE*, CompileUtf8, bool, bool) /home/p1umer/Git/gecko-dev/js/src/shell/js.cpp:1105:10
    #16 0x57f8170e in Process(JSContext*, char const*, bool, FileKind) /home/p1umer/Git/gecko-dev/js/src/shell/js.cpp
    #17 0x57ed112a in ProcessArgs(JSContext*, js::cli::OptionParser*) /home/p1umer/Git/gecko-dev/js/src/shell/js.cpp:10747:10
    #18 0x57ed112a in Shell(JSContext*, js::cli::OptionParser*) /home/p1umer/Git/gecko-dev/js/src/shell/js.cpp:10971:12
    #19 0x57ebf4eb in main /home/p1umer/Git/gecko-dev/js/src/shell/js.cpp:11403:12
    #20 0xf79e3ed4 in __libc_start_main /build/glibc-9vW8qm/glibc-2.31/csu/../csu/libc-start.c:308:16

AddressSanitizer can not provide additional info.
SUMMARY: AddressSanitizer: SEGV (<unknown module>)
==1641316==ABORTING

Comment 1

[P1umer]

I also got another sample(poc2.js), which can trigger a SEGV at a different address.

Comment 2

[P1umer]

Attachment: poc2.js

Comment 3

[P1umer]

The requied flags: --wasm-function-references --wasm-gc

Comment 4

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

The bug has a release status flag that shows some version of Firefox is affected, thus it will be considered confirmed.

Comment 5

[Julian Seward [:jseward]]

This is almost certainly the same underlying problem as bug 1843991, except it afflicts 32-bit x86 rather than arm64.

The segfault occurs because we have a wasm-gc null pointer access -- that should be caught and handled correctly. But the trap-site metadata refers to the wrong instruction, so the segfault isn't caught and so takes out the process. This is described in detail in bug 1843991 comment 10 and comment 11.

In this case we have a fault in the suspicious-looking bit of code

   0x5a46409a:  push   %ecx
   0x5a46409b:  mov    %edi,%ecx
=> 0x5a46409d:  mov    %cl,0x12(%eax)
   0x5a4640a0:  pop    %ecx

Ion wants to write the lowest 8 bits of %edi at 0x12(%eax), but x86 arcanery means it can't do that directly. So it moves the value first into %ecx and stores from there. %ecx is saved and restored around this. Result is that the insn that faults is not the first insn in what is almost certainly an "atomic" sequence (I mean, logically a single operation; nothing to do with threading) when viewed at some level in the assembler hierarchy. And indeed we see this:

void CodeGenerator::visitWasmStoreSlot(LWasmStoreSlot* ins) {
  // [..]
  EmitSignalNullCheckTrapSite(masm, ins); <------ records the buffer offset
  switch (type) {
    case MIRType::Int32:
      switch (narrowingOp) {
        case MNarrowingOp::None:
          masm.store32(src.gpr(), addr);
          break;
        case MNarrowingOp::To16:
          masm.store16(src.gpr(), addr);
          break;
        case MNarrowingOp::To8:
          masm.store8(src.gpr(), addr); <------ generates the faulting sequence
          break;
        default:
          MOZ_CRASH();
      }
      break;

where store8() is

template <typename T>
void store8(Register src, const T& dest) {
  AutoEnsureByteRegister ensure(this, dest, src);
  movb(ensure.reg(), Operand(dest));
}

which confirms the analysis. AutoEnsureByteRegister presumably produces the push and pop of %ecx, hence causing the movb not to be at the start of the sequence.

The discussion in bug 1843991 applies equally here.

Comment 6

[Andrew McCreight [:mccr8]]

Null derefs aren't sec issues, so I'm unhiding this.

Comment 7

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

The severity field is not set for this bug.
:rhunt, could you have a look please?

For more information, please visit BugBot documentation.

Comment 8

[Julian Seward [:jseward]]

Closing this on the basis that it has been fixed by the landing of bug
1846474. Unfortunately I can no longer run the test case to verify that,
since the test case uses the pre-final wasm-gc instruction encoding, and SM
recently changed to the final encoding.

If the bug still exists, please feel free to re-open. @P1umer, thank you for
finding and reporting this.