Closed Bug 1435525 Opened 2 years ago Closed 2 years ago

Controllable large allocation in Wasm through [@ js::jit::AssemblerBuffer::reserve]

Categories

(Core :: JavaScript Engine, defect, P1, critical)

x86_64
Linux
defect

Tracking

()

RESOLVED FIXED
mozilla60
Tracking Status
firefox-esr52 --- unaffected
firefox58 --- wontfix
firefox59 --- wontfix
firefox60 --- fixed

People

(Reporter: decoder, Assigned: luke)

References

(Blocks 1 open bug)

Details

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

Attachments

(2 files, 1 obsolete file)

The attached binary WebAssembly testcase causes a huge allocation on mozilla-inbound revision 39d76cb8c73e (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:

==1651==ERROR: AddressSanitizer failed to allocate 0x400002000 (17179877376) bytes of LargeMmapAllocator (error code: 12)
==1651==Process memory map follows:
	0x000000400000-0x000003374000	js
	[...]
	0xffffffffff600000-0xffffffffff601000	[vsyscall]
==1651==End of process memory map.
==1651==AddressSanitizer CHECK failed: compiler-rt/lib/sanitizer_common/sanitizer_common.cc:120 "((0 && "unable to mmap")) != (0)" (0x0, 0x0)
    #0 0x51ad7f in __asan::AsanCheckFailed(char const*, int, char const*, unsigned long long, unsigned long long) (js+0x51ad7f)
    #1 0x534c45 in __sanitizer::CheckFailed(char const*, int, char const*, unsigned long long, unsigned long long) (js+0x534c45)
    #2 0x524a71 in __sanitizer::ReportMmapFailureAndDie(unsigned long, char const*, char const*, int, bool) (js+0x524a71)
    #3 0x52e1c6 in __sanitizer::MmapOrDie(unsigned long, char const*, bool) (js+0x52e1c6)
    #4 0x462e3f in __asan::asan_malloc(unsigned long, __sanitizer::BufferedStackTrace*) (js+0x462e3f)
    #5 0x510184 in __interceptor_malloc (js+0x510184)
    #6 0xcac47c in js_malloc(unsigned long) dist/include/js/Utility.h:387:12
    #7 0xcac47c in unsigned char* js_pod_malloc<unsigned char>(unsigned long) dist/include/js/Utility.h:577
    #8 0xcac47c in unsigned char* js::SystemAllocPolicy::maybe_pod_malloc<unsigned char>(unsigned long) js/src/jsalloc.h:37
    #9 0xcac47c in unsigned char* js::SystemAllocPolicy::pod_malloc<unsigned char>(unsigned long) js/src/jsalloc.h:42
    #10 0xcac47c in mozilla::Vector<unsigned char, 256ul, js::SystemAllocPolicy>::convertToHeapStorage(unsigned long) dist/include/mozilla/Vector.h:959
    #11 0xcab551 in mozilla::Vector<unsigned char, 256ul, js::SystemAllocPolicy>::growStorageBy(unsigned long) dist/include/mozilla/Vector.h:1050:12
    #12 0xcae50f in mozilla::Vector<unsigned char, 256ul, js::SystemAllocPolicy>::reserve(unsigned long) dist/include/mozilla/Vector.h:1112:9
    #13 0x26185c7 in js::jit::AssemblerBuffer::reserve(unsigned long) js/src/jit/x86-shared/AssemblerBuffer-x86-shared.h:130:39
    #14 0x26185c7 in js::jit::X86Encoding::BaseAssembler::X86InstructionFormatter::reserve(unsigned long) js/src/jit/x86-shared/BaseAssembler-x86-shared.h:5165
    #15 0x26185c7 in js::jit::X86Encoding::BaseAssembler::reserve(unsigned long) js/src/jit/x86-shared/BaseAssembler-x86-shared.h:60
    #16 0x26185c7 in js::jit::AssemblerX86Shared::reserve(unsigned long) js/src/jit/x86-shared/Assembler-x86-shared.h:407
    #17 0x26185c7 in js::wasm::ModuleGenerator::init(js::wasm::Metadata*) js/src/wasm/WasmGenerator.cpp:216
    #18 0x261687b in js::wasm::CompileBuffer(js::wasm::CompileArgs const&, js::wasm::ShareableBytes const&, mozilla::UniquePtr<char [], JS::FreePolicy>*) js/src/wasm/WasmCompile.cpp:432:13
    #19 0x2700f8e in js::wasm::Eval(JSContext*, JS::Handle<js::TypedArrayObject*>, JS::Handle<JSObject*>, JS::MutableHandle<js::WasmInstanceObject*>) js/src/wasm/WasmJS.cpp:316:27
    #20 0x5c83db in WasmLoop(JSContext*, unsigned int, JS::Value*) js/src/shell/js.cpp:6748:14
    #21 0x8e20f5 in js::CallJSNative(JSContext*, bool (*)(JSContext*, unsigned int, JS::Value*), JS::CallArgs const&) js/src/jscntxtinlines.h:291:15
[...]

I stepped into this with GDB and found that js_malloc is indeed called with 17179877376 (17 GB). We shouldn't have such large allocations controllable by content anywhere and it makes fuzzing particularly hard as well. Marking fuzzblocker.
Attached file Testcase
Right, so this is a malformed wasm with a (bogus) size allocation for the code section of 3.5GB, and our estimator of code section size inflates this by the usual factor of 3+, leading us to try to reserve almost 14GB.

One the one hand we could classify this as a non-security-sensitive failure to sanitize user input.  On the other hand the only agreed upon upper limit we have on code section size right now is very large (1000000 functions * max 7654321 bytes per function, ie 7TB or so, https://github.com/WebAssembly/design/issues/1138#issuecomment-337766850).

For streaming compilation we don't want to wait for the entire code section to be available before reserving the machine code output buffer.  So we have a couple strategies available to us here:

- we introduce a cap on the code section size that's lower than 7TB (low complexity)
- we introduce a cap on the machine code buffer size that's lower than 17GB (low complexity), say, 1GB on 32-bit systems and 2GB on 64-bit systems (low complexity)
- if we encounter very large code sections we revert to piecemeal allocation of the output buffer (higher complexity)

Either way we should discuss with the wasm CG whether we should have limits not just on num functions and size per function, but overall bytecode size.  I'll make a note on that ticket.
Assignee: nobody → lhansen
Status: NEW → ASSIGNED
(In reply to Lars T Hansen [:lth] from comment #2)
> - we introduce a cap on the machine code buffer size that's lower than 17GB
> (low complexity), say, 1GB on 32-bit systems and 2GB on 64-bit systems (low
> complexity)

Note that we have a limit of 140 MB of executable code per process on 32-bit, 1 GB on 64-bit. So IIUC having a limit a bit smaller than that makes sense...
(In reply to Jan de Mooij [:jandem] from comment #3)
> (In reply to Lars T Hansen [:lth] from comment #2)
> > - we introduce a cap on the machine code buffer size that's lower than 17GB
> > (low complexity), say, 1GB on 32-bit systems and 2GB on 64-bit systems (low
> > complexity)
> 
> Note that we have a limit of 140 MB of executable code per process on
> 32-bit, 1 GB on 64-bit. So IIUC having a limit a bit smaller than that makes
> sense...

An excellent point, this will make it easier to justify the simple fix.

(/me wonders if that limit is only applied when we try to copy into executable memory / set permission bits, or if it's also applied when the assembler tries to expand / create more buffers.  Should investigate that.)
Funny.  We do have a limit like this in the wasm compiler, using MaxCodeBytesPerProcess, but only on 32-bit.
... and then only to determine whether to tier or not.
So, this probably is only an issue on x86 and x64.  On ARM, ARM64, MIPS, and MIPS64 the reserve() call that allocates memory ignores the request size because memory is allocated in chunks anyway, and OOM is flagged properly later as we discover that memory is not available.  Only on x86/x64 will we try to allocate a contiguous area of memory.
Incidentally, we already have a constant MaxModuleBytes (1gb), but we only check this against the code section size in the streaming case (in consumeChunk()).  If we moved this check to right after the startSection(SectionId::Code) in wasm::DecodeModuleEnvironment() then it would apply to the non-streaming case (incl comment 0).  Probably should rename to MaxCodeSectionSize...

But furthermore it makes sense to clamp the masm reservation size to MaxCodeBytesPerProcess.  I can write the patch if noone beats me to it.
Go for it.  I had a patch that changed the assembler guts (best pinch point, I thought) but it changed behavior for oomTest and became a boondoggle.
Assignee: lhansen → luke
Attached patch clamp-code-size (obsolete) — Splinter Review
Attachment #8948436 - Flags: review?(lhansen)
Comment on attachment 8948436 [details] [diff] [review]
clamp-code-size

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

::: js/src/wasm/WasmJS.cpp
@@ -2519,5 @@
>                  envBytes_.shrinkTo(codeSection_.start);
>  
> -            if (codeSection_.size > MaxModuleBytes)
> -                return rejectAndDestroyBeforeHelperThreadStarted(JSMSG_OUT_OF_MEMORY);
> -

I'm failing to convince myself that this change is desirable.  We're not parsing the ModuleEnvironment until later, so don't we want this check to guard the resize() on the next line?
Status: ASSIGNED → NEW
Priority: -- → P1
(In reply to Lars T Hansen [:lth] from comment #11)
Excellent catch; we've only called StartsCodeSection() successfully, not DecodeModuleEnvironment().
Attached patch clamp-code-sizeSplinter Review
Attachment #8948436 - Attachment is obsolete: true
Attachment #8948436 - Flags: review?(lhansen)
Attachment #8949038 - Flags: review?(lhansen)
Attachment #8949038 - Flags: review?(lhansen) → review+
Pushed by lwagner@mozilla.com:
https://hg.mozilla.org/integration/mozilla-inbound/rev/687cee80487f
Baldr: eagerly reject too-big code section sizes and clamp masm reservation size (r=lth)
https://hg.mozilla.org/mozilla-central/rev/687cee80487f
Status: NEW → RESOLVED
Closed: 2 years ago
Resolution: --- → FIXED
Target Milestone: --- → mozilla60
How far back does this go?
Flags: needinfo?(luke)
This holds back fuzzers and allows DoS attacks, but I don't think it's a bug that requires backport.
Flags: needinfo?(luke)
It's not completely obvious that uplifting is important.  So far as we know there's a strong relationship between bytecode size and executable code size, so if the .wasm tells us we need N bytes of executable memory then unless the .wasm is lying (in which case it will be rejected anyway, eventually) we probably will need those N bytes and we should resize the array to N, so as to avoid requiring even more memory down the line when we need to expand it.

I guess one could make an argument that if we know we will need more executable memory than we can hope to provide for a wasm that is not lying, we can bring up a better error message, or we can signal the error sooner.
You need to log in before you can comment on or make changes to this bug.