Last Comment Bug 992968 - AddressSanitizer: heap-buffer-overflow [@ js::jit::CodeGeneratorShared::allocateCache]
: AddressSanitizer: heap-buffer-overflow [@ js::jit::CodeGeneratorShared::alloc...
Status: VERIFIED FIXED
[adv-main29+][adv-esr24.5+]
: csectype-bounds, sec-high
Product: Core
Classification: Components
Component: JavaScript Engine: JIT (show other bugs)
: Trunk
: x86_64 Linux
: -- critical (vote)
: mozilla31
Assigned To: Nicolas B. Pierron [:nbp]
:
: Hannes Verschore [:h4writer]
Mentors:
Depends on:
Blocks: langfuzz 912928
  Show dependency treegraph
 
Reported: 2014-04-07 08:46 PDT by Christian Holler (:decoder)
Modified: 2015-08-30 12:00 PDT (History)
10 users (show)
See Also:
Crash Signature:
(edit)
QA Whiteboard:
Iteration: ---
Points: ---
Has Regression Range: ---
Has STR: ---
wontfix
+
fixed
+
fixed
+
fixed
+
fixed
unaffected
unaffected
fixed
fixed
fixed
fixed
fixed


Attachments
OOM in allocateCache (1.93 KB, patch)
2014-04-07 10:58 PDT, Nicolas B. Pierron [:nbp]
efaustbmo: review+
sledru: approval‑mozilla‑aurora+
sledru: approval‑mozilla‑beta+
abillings: sec‑approval+
Details | Diff | Splinter Review
bug992968-esr24.patch (1.94 KB, patch)
2014-04-16 05:32 PDT, Nicolas B. Pierron [:nbp]
nicolas.b.pierron: review+
sledru: approval‑mozilla‑esr24+
Details | Diff | Splinter Review

Description User image Christian Holler (:decoder) 2014-04-07 08:46:10 PDT
The following trace came up while doing OOM fuzzing on mozilla-central revision :


==46451==ERROR: AddressSanitizer: heap-buffer-overflow on address 0x611000063ec7 at pc 0x8a0820 bp 0x7f53ff5718f0 sp 0x7f53ff5718e8
WRITE of size 32 at 0x611000063ec7 thread T3 (Analysis Helper)
out of memory
out of memory
    #0 0x8a081f in unsigned long js::jit::CodeGeneratorShared::allocateCache<js::jit::NameIC>(js::jit::NameIC const&) js/src/jit/IonCaches.h:127
    #1 0x8789f7 in js::jit::CodeGenerator::generateBody() js/src/jit/CodeGenerator.cpp:3240
    #2 0x89ad50 in js::jit::CodeGenerator::generate() js/src/jit/CodeGenerator.cpp:6235
    #3 0x969923 in js::jit::GenerateCode(js::jit::MIRGenerator*, js::jit::LIRGraph*, js::jit::MacroAssembler*) js/src/jit/Ion.cpp:1598
    #4 0xfc3f8f in js::WorkerThread::handleIonWorkload() js/src/jsworkers.cpp:798
    #5 0xfc3060 in js::WorkerThread::threadLoop() js/src/jsworkers.cpp:1034
    #6 0x10fe1a8 in nspr::Thread::ThreadRoutine(void*) js/src/vm/PosixNSPR.cpp:45
    #7 0x7f5405482e99 in start_thread /build/buildd/eglibc-2.15/nptl/pthread_create.c:308
    #8 0x7f54043763fc in ?? /build/buildd/eglibc-2.15/misc/../sysdeps/unix/sysv/linux/x86_64/clone.S:112

0x611000063ec7 is located 7 bytes to the right of 256-byte region [0x611000063dc0,0x611000063ec0)
allocated by thread T3 (Analysis Helper) here:
    #0 0x461116 in __interceptor_realloc _asan_rtl_
    #1 0x8f269a in js_realloc(void*, unsigned long) js/src/opt64asan-oom/js/src/../../dist/include/js/Utility.h:115
    #2 0x8f1cd4 in mozilla::VectorBase<unsigned char, 0ul, js::SystemAllocPolicy, js::Vector<unsigned char, 0ul, js::SystemAllocPolicy> >::appendN(unsigned char const&, unsigned long) js/src/opt64asan-oom/js/src/../../dist/include/mozilla/Vector.h:925
    #3 0x8a0422 in unsigned long js::jit::CodeGeneratorShared::allocateCache<js::jit::NameIC>(js::jit::NameIC const&) js/src/jit/shared/CodeGenerator-shared.h:251
    #4 0x8789f7 in js::jit::CodeGenerator::generateBody() js/src/jit/CodeGenerator.cpp:3240
    #5 0x89ad50 in js::jit::CodeGenerator::generate() js/src/jit/CodeGenerator.cpp:6235
    #6 0x969923 in js::jit::GenerateCode(js::jit::MIRGenerator*, js::jit::LIRGraph*, js::jit::MacroAssembler*) js/src/jit/Ion.cpp:1598
    #7 0xfc3f8f in js::WorkerThread::handleIonWorkload() js/src/jsworkers.cpp:798
    #8 0xfc3060 in js::WorkerThread::threadLoop() js/src/jsworkers.cpp:1034
    #9 0x10fe1a8 in nspr::Thread::ThreadRoutine(void*) js/src/vm/PosixNSPR.cpp:45
    #10 0x7f5405482e99 in start_thread /build/buildd/eglibc-2.15/nptl/pthread_create.c:308

Thread T3 (Analysis Helper) created by T0 here:
    #0 0x424c06 in __interceptor_pthread_create _asan_rtl_
    #1 0x10fe427 in PR_CreateThread(PRThreadType, void (*)(void*), void*, PRThreadPriority, PRThreadScope, PRThreadState, unsigned int) js/src/vm/PosixNSPR.cpp:108
    #2 0xfbbe15 in js::GlobalWorkerThreadState::ensureInitialized() js/src/jsworkers.cpp:437
    #3 0xfc4e28 in js::EnsureWorkerThreadsInitialized(js::ExclusiveContext*) js/src/jsworkers.cpp:44
    #4 0xea90f3 in js::ScriptSource::setSourceCopy(js::ExclusiveContext*, char16_t const*, unsigned int, bool, js::SourceCompressionTask*) js/src/jsscript.cpp:1611
    #5 0x549af8 in js::frontend::CompileScript(js::ExclusiveContext*, js::LifoAlloc*, JS::Handle<JSObject*>, JS::Handle<JSScript*>, JS::ReadOnlyCompileOptions const&, char16_t const*, unsigned long, JSString*, unsigned int, js::SourceCompressionTask*) js/src/frontend/BytecodeCompiler.cpp:225
    #6 0xcdd2e5 in JS::Compile(JSContext*, JS::Handle<JSObject*>, JS::ReadOnlyCompileOptions const&, char16_t const*, unsigned long) js/src/jsapi.cpp:4470
    #7 0xcdd854 in mozilla::VectorBase<char, 8ul, js::TempAllocPolicy, js::Vector<char, 8ul, js::TempAllocPolicy> >::begin() js/src/jsapi.cpp:4485
    #8 0x486317 in RunFile(JSContext*, JS::Handle<JSObject*>, char const*, _IO_FILE*, bool) js/src/shell/js.cpp:447
    #9 0x47ea00 in ProcessArgs(JSContext*, JSObject*, js::cli::OptionParser*) js/src/shell/js.cpp:5732
    #10 0x7f54042a376c in __libc_start_main /build/buildd/eglibc-2.15/csu/libc-start.c:226

SUMMARY: AddressSanitizer: heap-buffer-overflow ??:0 ??
Shadow bytes around the buggy address:
  0x0c2280004780: 00 00 00 00 00 00 00 00 fa fa fa fa fa fa fa fa
  0x0c2280004790: fd fd fd fd fd fd fd fd fd fd fd fd fd fd fd fd
  0x0c22800047a0: fd fd fd fd fd fd fd fd fd fd fd fd fd fd fd fd
  0x0c22800047b0: fa fa fa fa fa fa fa fa 00 00 00 00 00 00 00 00
  0x0c22800047c0: 00 00 00 00 00 00 00 00 00 00 00 00 00 00 00 00
=>0x0c22800047d0: 00 00 00 00 00 00 00 00[fa]fa fa fa fa fa fa fa
  0x0c22800047e0: fd fd fd fd fd fd fd fd fd fd fd fd fd fd fd fd
  0x0c22800047f0: fd fd fd fd fd fd fd fd fd fd fd fd fd fd fd fd
  0x0c2280004800: fa fa fa fa fa fa fa fa fa fa fa fa fa fa fa fa
  0x0c2280004810: fa fa fa fa fa fa fa fa fa fa fa fa fa fa fa fa
  0x0c2280004820: fa fa fa fa fa fa fa fa fa fa fa fa fa fa fa fa
Shadow byte legend (one shadow byte represents 8 application bytes):
  Addressable:           00
  Partially addressable: 01 02 03 04 05 06 07
  Heap left redzone:       fa
  Freed heap region:       fd
==46451==ABORTING


No test is available for this issue, reproducing it only yields an intended OOM crash. Thread scheduling is probably the reason that it doesn't reproduce properly.
Comment 1 User image Nicolas B. Pierron [:nbp] 2014-04-07 10:58:58 PDT
Created attachment 8402790 [details] [diff] [review]
OOM in allocateCache

All OOM flags are well propagated, but we do not check the oom flag before doing a placement new on the reserved memory allocated with the SystemAllocator.

This patch check the oom flags and return an error code which always flow into addCache, which is instrumented to check this error code and make the compilation fail in case of OOM.

Such issue is extremely hard to reproduce as this is used for Ion compilations only, which is run on an helper thread.  It might be easier to reproduce with --ion-parallel-compile=off.
Comment 2 User image Nicolas B. Pierron [:nbp] 2014-04-07 11:14:45 PDT
This issue is coming from Bug 814823.
Comment 3 User image Eric Faust [:efaust] 2014-04-11 12:31:18 PDT
Comment on attachment 8402790 [details] [diff] [review]
OOM in allocateCache

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

Yep. Oops. r=me
Comment 4 User image Nicolas B. Pierron [:nbp] 2014-04-14 02:27:11 PDT
Comment on attachment 8402790 [details] [diff] [review]
OOM in allocateCache

[Security approval request comment]
> How easily could an exploit be constructed based on the patch?

 - On a multiple core computer, this is hard to reproduce as this involve doing an OOM in a separated thread which is not controlled by us.

 - On a Single core device, such as some low end phones.  This is more likely to be exploitable, but still hard to use for an exploit as the content of the memory written is hard to control from an attacker perspective (register allocation)

 Note that OOM are still handled and prevent using these data with executable flags.

> Do comments in the patch, the check-in comment, or tests included in the patch paint a bulls-eye on the security problem?

The patch it-self highlight that we did not handled OOMs.

> Which older supported branches are affected by this flaw?

All except b2g18.

> If not all supported branches, which bug introduced the flaw?

Bug 814823

> Do you have backports for the affected branches? If not, how different, hard to create, and risky will they be?

I do not have backports yet.
This should be trivial to make the same fixes, as the patch would not apply because of modification of near-by lines.

> How likely is this patch to cause regressions; how much testing does it need?

Unlikely.
Comment 5 User image Al Billings [:abillings] 2014-04-14 09:59:54 PDT
Comment on attachment 8402790 [details] [diff] [review]
OOM in allocateCache

sec-approval+. We should get this on Aurora, Beta, and ESR24. Please make and nominate patches ASAP as we're running out of time.
Comment 6 User image Nicolas B. Pierron [:nbp] 2014-04-16 05:30:32 PDT
Comment on attachment 8402790 [details] [diff] [review]
OOM in allocateCache

[Approval Request Comment]
(see comment 4)
String or IDL/UUID changes made by this patch:  N/A
Comment 7 User image Nicolas B. Pierron [:nbp] 2014-04-16 05:32:27 PDT
Created attachment 8407481 [details] [diff] [review]
bug992968-esr24.patch

[Approval Request Comment]
If this is not a sec:{high,crit} bug, please state case for ESR consideration:
  sec-high
User impact if declined: 
Fix Landed on Version:
Risk to taking this patch (and alternatives if risky): 
  (see comment 4)
String or UUID changes made by this patch:
  N/A

See https://wiki.mozilla.org/Release_Management/ESR_Landing_Process for more info.
Comment 8 User image Nicolas B. Pierron [:nbp] 2014-04-16 05:35:03 PDT
(In reply to Al Billings [:abillings] from comment #5)
> sec-approval+. We should get this on Aurora, Beta, and ESR24. Please make
> and nominate patches ASAP as we're running out of time.

I am not sure to understand, should I land the patch right now, and wait for approvals on other branches or wait for approval of all branches before landing on inbound?
Comment 9 User image Ryan VanderMeulen [:RyanVM] 2014-04-16 06:10:38 PDT
Comment on attachment 8402790 [details] [diff] [review]
OOM in allocateCache

Land on inbound now. This won't be uplifted to the release branches until it's on trunk.
Comment 10 User image Nicolas B. Pierron [:nbp] 2014-04-16 06:19:00 PDT
https://hg.mozilla.org/integration/mozilla-inbound/rev/1a12d31c2289
Comment 11 User image Ryan VanderMeulen [:RyanVM] 2014-04-16 19:21:01 PDT
https://hg.mozilla.org/mozilla-central/rev/1a12d31c2289
Comment 13 User image Matt Wobensmith [:mwobensmith][:matt:] 2014-04-22 14:35:58 PDT
Hi decoder - can you verify that this was fixed before we release? Thanks!
Comment 14 User image Christian Holler (:decoder) 2014-04-23 07:09:33 PDT
I'm not seeing this anymore popping up, so I assume it's fixed. I cannot verify this though for our release builds, since this is an OOM issue and we don't even have a reliable test.

Note You need to log in before you can comment on or make changes to this bug.