Closed Bug 867597 Opened 7 years ago Closed 6 years ago
Monkey: ARM hwcaps detection depends on uninitialised garbage on the stack
js/src/ion/arm/Architecture-arm.cpp:64 ibid has this #elif defined(WTF_OS_ANDROID) || defined(MOZ_B2G) FILE *fp = fopen("/proc/cpuinfo", "r"); if (!fp) return false; char buf; fread(buf, sizeof(char), sizeof(buf), fp); fclose(fp); if (strstr(buf, "vfp")) flags |= HWCAP_VFP; if (strstr(buf, "vfpv3")) flags |= HWCAP_VFPv3; if (strstr(buf, "vfpv3d16")) flags |= HWCAP_VFPv3D16; if (strstr(buf, "vfpv4")) flags |= HWCAP_VFPv4; if (strstr(buf, "idiva")) flags |= HWCAP_IDIVA; if (strstr(buf, "idivt")) flags |= HWCAP_IDIVT; if (strstr(buf, "neon")) flags |= HWCAP_NEON; // not part of the HWCAP flag, but I need to know this, and we're not using // that bit, so... I'm using it if (strstr(buf, "ARMv7")) flags |= HWCAP_ARMv7; isSet = true; return flags; #endif viz. open /proc/cpuinfo, read arbitrary amount stuff into buf, not zero terminated, and then do strstr on the buf. Since in my test case, the file fits in buf (as one would expect), it winds up doing strstr on the garbage left over in buf. Hence (1) for the strings that don't appear /proc/cpuinfo (eg, vfpv4), the do/don't-we-have-this-feature result is random, and (2) if all of the garbage in buf doesn't contain a zero byte, we'll overrun the array.
Note that Architecture-arm.cpp:81 is this if (strstr(buf, "vfpv4")) and that's the first string that isn't in /proc/cpuinfo on this tablet, hence supporting the analysis in the previous comment. Conditional jump or move depends on uninitialised value(s) at 0x4808EDC: strstr (/home/sewardj/VgTRUNK/trunk-droid/memcheck/mc_replace_strmem.c:1411) by 0x2B20055F: js::ion::getFlags() [clone .part.1] (js/src/ion/arm/Architecture-arm.cpp:81) by 0x2B200677: js::ion::hasMOVWT() (js/src/ion/arm/Architecture-arm.cpp:40) by 0x2B200DE9: js::ion::MacroAssemblerARM::ma_alu(js::ion::Register, js::ion::Imm32, js::ion::Register, js::ion::ALUOp, js::ion::SetCond_, js::ion::Assembler::Condition) (js/src/ion/arm/MacroAssembler-arm.cpp:212) by 0x2B207A35: js::ion::MacroAssemblerARMCompat::callWithABI(void*, js::ion::MacroAssemblerARMCompat::Result) (js/src/ion/arm/MacroAssembler-arm.cpp:370) by 0x2B1F1DA3: GenerateBailoutThunk(js::ion::MacroAssembler&, unsigned int) [clone .constprop.68] (js/src/ion/IonMacroAssembler.h:645) by 0x2B1F7169: js::ion::IonRuntime::generateBailoutTable(JSContext*, unsigned int) (js/src/ion/arm/Trampoline-arm.cpp:566) by 0x2B162A5F: js::ion::IonRuntime::initialize(JSContext*) (js/src/ion/Ion.cpp:210) by 0x2AFBB095: JSRuntime::createIonRuntime(JSContext*) (js/src/jscompartment.cpp:120) by 0x2AFBB17F: JSCompartment::ensureIonCompartmentExists(JSContext*) (js/src/jscntxt.h:789) by 0x2B15AEC7: js::ion::CanEnterBaselineJIT(JSContext*, JSScript*, js::StackFrame*, bool) (js/src/ion/BaselineJIT.cpp:256) by 0x2B002AA1: js::RunScript(JSContext*, js::StackFrame*) (js/src/jsinterp.cpp:357) by 0x2B003997: js::Execute(JSContext*, JS::Handle<JSScript*>, JSObject&, JS::Value*) (js/src/jsinterp.cpp:573) by 0x2AF9DB7D: JS_ExecuteScript(JSContext*, JSObject*, JSScript*, JS::Value*) (js/src/jsapi.cpp:5603) by 0x2A5F82F3: nsJSContext::ExecuteScript(JSScript*, JSObject*) (dom/base/nsJSEnvironment.cpp:1436) by 0x2A5E5DC9: mozilla::dom::XULDocument::ExecuteScript(nsIScriptContext*, JSScript*) (content/xul/document/src/XULDocument.cpp:3658) by 0x2A5E5E1F: mozilla::dom::XULDocument::ExecuteScript(nsXULPrototypeScript*) (content/xul/document/src/XULDocument.cpp:3678) by 0x2A5E9F09: mozilla::dom::XULDocument::OnStreamComplete(nsIStreamLoader*, nsISupports*, tag_nsresult, unsigned int, unsigned char const*) (content/xul/document/src/XULDocument.cpp:3557) by 0x2A22D723: nsStreamLoader::OnStopRequest(nsIRequest*, nsISupports*, tag_nsresult) (netwerk/base/src/nsStreamLoader.cpp:101) by 0x2A303F97: nsJARChannel::OnStopRequest(nsIRequest*, nsISupports*, tag_nsresult) (modules/libjar/nsJARChannel.cpp:982) by 0x2A219D57: nsInputStreamPump::OnStateStop() (netwerk/base/src/nsInputStreamPump.cpp:555) by 0x2A219DE7: nsInputStreamPump::OnInputStreamReady(nsIAsyncInputStream*) (netwerk/base/src/nsInputStreamPump.cpp:375) by 0x2AB709CF: nsInputStreamReadyEvent::Run() (xpcom/io/nsStreamUtils.cpp:160) by 0x2AB7BA1B: nsThread::ProcessNextEvent(bool, bool*) (xpcom/threads/nsThread.cpp:627) Uninitialised value was created by a stack allocation at 0x2B2004E2: js::ion::getFlags() [clone .part.1] (js/src/ion/arm/Architecture-arm.cpp:35)
This strikes me as safer: char buf; memset(buf, 0, sizeof(buf)); fread(buf, sizeof(char), sizeof(buf)-1, fp);
Hm, looking at that code and talking to sewardj on IRC, I wonder if these string searches could also be causing the Baseline VFP crashes with HTC Wildfire (e.g. the string "vfp" appearing somewhere else in /proc/cpuinfo)?
Well, can we get a /proc/cpuinfo dump for HTC Wildfire from anyone?
If you can find one on crash-stats the full contents of /proc/cpuinfo is actually present in the minidump, I can dig it out for you if you'd like. (Or you can use minidump_dump from Breakpad to see it if you have access to dump files.)
I just looked at one of the dumps: Stream MD_LINUX_CPU_INFO: Processor : ARMv6-compatible processor rev 2 (v6l) BogoMIPS : 708.05 Features : swp half thumb fastmult edsp java CPU implementer : 0x41 CPU architecture: 6TEJ CPU variant : 0x1 CPU part : 0xb36 CPU revision : 2 Hardware : buzz Revision : 0081 Serial : 0000000000000000 I don't see the string vfp anywhere in the minidump_dump output, so this must be another issue..
Summary: IM's ARM hwcaps detection depends on uninitialised garbage on the stack → IonMonkey: ARM hwcaps detection depends on uninitialised garbage on the stack
I'm assuming b2g18 is "unaffected" because ionmonkey is disabled, but the code does exist. Is this a feature that is used regardless?
Julian, how much of a security issue could this be? I'm not sure what hwcaps is.
hwcaps is short for Hardware Capabilities. I don't think this is a big security deal, since in the worst case we can overread the stack only if it is possible to replace /proc/cpuinfo with a file that is at least 1024 bytes long (or otherwise cause it to be so). There is also the possibility that we wind up generating instructions that the processor can't execute, if by chance garbage on the stack contains a string along the lines of "vfpv4" or others that the routine checks for. Basically it's a correctness issue, and should definitely be fixed. Comment 2 has what I think is a minimal fix (untested).
Who can we assign this to?
Minimal fix, which at least (1) ensures that we can't overrun the stack allocated buffer, and (2) stops Valgrind complaining.
Attachment #762404 - Flags: review?(mrosenberg)
Attachment #762404 - Flags: review?(mrosenberg) → review+
Julian, can you land your fix? Thanks!
Assignee: general → jseward
Status: NEW → ASSIGNED
Marty, since Julian is not responding, can you make sure this gets fixed? Thanks!
I'll land it; sorry for the delay. Currently waiting for it to look ok on try.
https://hg.mozilla.org/mozilla-central/rev/97feecba44a4 Can we land the test too please?
Status: ASSIGNED → RESOLVED
Closed: 6 years ago
Flags: needinfo?(mrosenberg) → in-testsuite?
Resolution: --- → FIXED
Target Milestone: --- → mozilla29
(In reply to Ryan VanderMeulen [:RyanVM UTC-5] from comment #16) > Can we land the test too please? This was detected by Valgrind during a normal startup of Firefox on Android. There is no test case.
Please nominate this for Aurora/Beta/b2g26/esr24 uplift.
Comment on attachment 762404 [details] [diff] [review] A minimal fix [Approval Request Comment] Bug caused by (feature/regressing bug #): N/A User impact if declined: Possible segfault due to overread of stack, possible SIGILL (although very unlikely) due to misidentification of hardware capabilities. Per comment 9, an attacker would need to be able to replace /proc/cpuinfo with a file > 1024 bytes to have any malicious effect. Testing completed (on m-c, etc.): Looked OK on try. Risk to taking this patch (and alternatives if risky): Minimal risk IMO. String or IDL/UUID changes made by this patch: none
Attachment #762404 - Flags: approval-mozilla-aurora?
Comment on attachment 762404 [details] [diff] [review] A minimal fix [Approval Request Comment] Same as comment 19.
Being an ARM-only sec-low, I don't think we need this on esr24, but B2G still sounds like a good idea.
Sec-low bugs are not normally backported to ESR branches without a specific reason.
Comment on attachment 762404 [details] [diff] [review] A minimal fix This doesn't qualify for ESR landing (nor is there a reason given to make an exception) see https://wiki.mozilla.org/Release_Management/ESR_Landing_Process
Attachment #762404 - Flags: approval-mozilla-esr24? → approval-mozilla-esr24-
FWIW, the b2g26 backport is green on Try.
Comment on attachment 762404 [details] [diff] [review] A minimal fix Minus on approval‑mozilla‑b2g26 as it is a low sec issue
Attachment #762404 - Flags: approval-mozilla-b2g26? → approval-mozilla-b2g26-
You need to log in before you can comment on or make changes to this bug.