Closed Bug 1461291 Opened 2 years ago Closed 2 years ago

[wasm] Crash [@ __sanitizer::internal_memmove] or Crash [@ __memmove_ssse3_back] through [@ js::wasm::Instance::memCopy]

Categories

(Core :: JavaScript Engine, defect, critical)

x86_64
Linux
defect
Not set
critical

Tracking

()

RESOLVED FIXED
mozilla62
Tracking Status
firefox-esr52 --- unaffected
firefox-esr60 --- unaffected
firefox60 --- unaffected
firefox61 --- unaffected
firefox62 --- fixed

People

(Reporter: decoder, Assigned: jseward)

References

(Blocks 1 open bug)

Details

(Keywords: crash, regression, testcase, Whiteboard: [fuzzblocker])

Crash Data

Attachments

(3 files, 1 obsolete file)

The attached binary WebAssembly testcase crashes on mozilla-inbound revision 84bf13246717 (build with --enable-posix-nspr-emulation --enable-valgrind --enable-gczeal --disable-tests --enable-address-sanitizer --disable-jemalloc --enable-optimize=-O2 --without-intl-api --enable-debug). To reproduce, you can run the following code in the JS shell:

var data = os.file.readFile(file, 'binary');
new WebAssembly.Instance(new WebAssembly.Module(data.buffer));



Backtrace:

==9260==ERROR: AddressSanitizer: SEGV on unknown address 0x7f8bc1be1fff (pc 0x0000005369f4 bp 0x7ffd6fb46730 sp 0x7ffd6fb45eb8 T0)
==9260==The signal is caused by a WRITE memory access.
    #0 0x5369f3 in __sanitizer::internal_memmove(void*, void const*, unsigned long) (/home/ubuntu/build/build/js+0x5369f3)
    #1 0x505afb in __asan_memmove (/home/ubuntu/build/build/js+0x505afb)
    #2 0x273dd1d in js::wasm::Instance::memCopy(js::wasm::Instance*, unsigned int, unsigned int, unsigned int) js/src/wasm/WasmInstance.cpp:427:13
    #3 0x124bb9349ed8  (<unknown module>)

AddressSanitizer can not provide additional info.
SUMMARY: AddressSanitizer: SEGV (/home/ubuntu/build/build/js+0x5369f3) in __sanitizer::internal_memmove(void*, void const*, unsigned long)
==9260==ABORTING


Marking s-s until triaged.
Attached file Testcase
This is an automated crash issue comment:

Summary: Crash [@ __memset_sse2]
Build version: mozilla-inbound revision 84bf13246717
Build flags: --enable-posix-nspr-emulation --enable-valgrind --enable-gczeal --disable-tests --enable-address-sanitizer --disable-jemalloc --enable-optimize=-O2 --without-intl-api --enable-debug
Runtime options: 

Testcase:

See attachment.

Backtrace:

==15771==ERROR: AddressSanitizer: SEGV on unknown address 0x7fdf64fb7fc1 (pc 0x7fdfe81e6a25 bp 0x7ffce84614b0 sp 0x7ffce8460c38 T0)
==15771==The signal is caused by a WRITE memory access.
    #0 0x7fdfe81e6a24  in __memset_sse2 (/lib/x86_64-linux-gnu/libc.so.6+0x172a24)
    #1 0x5057ae in __asan_memset (/home/ubuntu/build/build/js+0x5057ae)
    #2 0x273dfb1 in js::wasm::Instance::memFill(js::wasm::Instance*, unsigned int, unsigned int, unsigned int) js/src/wasm/WasmInstance.cpp:465:13
    #3 0x23ea370e3f38  (<unknown module>)

AddressSanitizer can not provide additional info.
SUMMARY: AddressSanitizer: SEGV (/lib/x86_64-linux-gnu/libc.so.6+0x172a24) 
==15771==ABORTING


Assuming this is the same bug, just with memFill instead of memCopy.
Attached file Testcase for comment 2
Julian, can you take a look?
Flags: needinfo?(jseward)
This can also crash in multiple more ways:

Assertion failure: byteLength % wasm::PageSize == 0, at wasm/WasmInstance.cpp:323
Assertion failure: mappedSize % gc::SystemPageSize() == 0, at vm/ArrayBufferObject.cpp:209
AddressSanitizer: negative-size-param: (size=-1) [@ __asan_memset] (with variable sizes).

There is also this stack, which I also believe is likely a dup to this bug?

=ERROR: AddressSanitizer: SEGV on unknown address 0x000000000000 (pc 0x000000606ce6 bp 0x7ffd50d96bb0 sp 0x7ffd50d96b90 T0)
    #0 0x606ce5 in JSObject::getClass() const js/src/vm/JSObject.h:114:16
    #1 0x606ce5 in js::NativeObject::getReservedSlot(unsigned int) const js/src/vm/NativeObject.h:1050
    #2 0x29277d5 in js::WasmMemoryObject::buffer() const js/src/wasm/WasmJS.cpp:1621:12
    #3 0x29277d5 in js::WasmMemoryObject::isShared() const js/src/wasm/WasmJS.cpp:1644
    #4 0x29277d5 in js::WasmMemoryObject::volatileMemoryLength() const js/src/wasm/WasmJS.cpp:1634
    #5 0x2868199 in js::wasm::Instance::memFill(js::wasm::Instance*, unsigned int, unsigned int, unsigned int) js/src/wasm/WasmInstance.cpp:442:28
    [...]
Whiteboard: [fuzzblocker]
Attached patch A possible fix (obsolete) — Splinter Review
Thanks for the test cases.

This fixes the segfaults in
  Instance::memCopy()'s call to memmove
and
  Instance::memFill()'s call to memset

These are both due to wraparound in the range checks, in the case where
(uint32_t)(byteOffset + len) == 0, for the relevant byteOffset and len
pairings.  I'll continue testing.

I didn't see any of the assertion failures in comment 5, though.
Flags: needinfo?(jseward)
As previous patch, but with commit message.
Attachment #8975738 - Attachment is obsolete: true
Attachment #8975791 - Flags: review?(lhansen)
Comment on attachment 8975791 [details] [diff] [review]
bug1461291-2.diff

Review of attachment 8975791 [details] [diff] [review]:
-----------------------------------------------------------------

Yeah, subtle.  Should have caught this in the previous review :(
Attachment #8975791 - Flags: review?(lhansen) → review+
Yeah, I should have spotted it too :(  The revised code is actually
a bit easier to reason about.
Can we land this testcase? The bug was trunk-only, so there shouldn't be any restrictions on doing so.
Assignee: nobody → jseward
Group: javascript-core-security → core-security-release
Flags: needinfo?(jseward)
Flags: in-testsuite?
(In reply to Ryan VanderMeulen [:RyanVM] from comment #12)
> Can we land this testcase? The bug was trunk-only, so there shouldn't be any
> restrictions on doing so.

The testcase is un-minimised, and requires a whole bunch of supporting wasm
binary files (IIUC).  So landing it as-is isn't viable.  Decoder, is this
something that's feasible/desirable to minimise?

Truth be told I could *probably* write a small testcase by hand, now I know
what the actual problem was.
Flags: needinfo?(jseward) → needinfo?(choller)
The testcase here is not unminimized, the tests attached are all standalone tests, no support files are needed.
Flags: needinfo?(choller)
Group: core-security-release
You need to log in before you can comment on or make changes to this bug.