Closed Bug 867597 Opened 7 years ago Closed 6 years ago

IonMonkey: ARM hwcaps detection depends on uninitialised garbage on the stack

Categories

(Core :: JavaScript Engine: JIT, defect)

ARM
Android
defect
Not set

Tracking

()

RESOLVED FIXED
mozilla29
Tracking Status
firefox26 --- wontfix
firefox27 --- fixed
firefox28 --- fixed
firefox29 --- fixed
firefox-esr17 --- unaffected
firefox-esr24 --- wontfix
b2g18 --- unaffected
b2g-v1.1hd --- unaffected
b2g-v1.2 --- wontfix
b2g-v1.3 --- fixed
b2g-v1.3T --- fixed
b2g-v1.4 --- fixed

People

(Reporter: jseward, Assigned: jseward)

Details

(Keywords: sec-low, Whiteboard: [adv-main27+])

Attachments

(1 file)

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[1024];
    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[1024];
    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?
Flags: sec-bounty?
Julian, how much of a security issue could this be?  I'm not sure what hwcaps is.
Flags: needinfo?(jseward)
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).
Flags: needinfo?(jseward)
Who can we assign this to?
Keywords: sec-low
Attached patch A minimal fixSplinter Review
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
Component: JavaScript Engine → JavaScript Engine: JIT
Flags: needinfo?(jseward)
Marty, since Julian is not responding, can you make sure this gets fixed? Thanks!
Flags: needinfo?(mrosenberg)
I'll land it; sorry for the delay.  Currently waiting for it to look ok on try.
Flags: needinfo?(jseward)
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.
Flags: in-testsuite? → in-testsuite+
Please nominate this for Aurora/Beta/b2g26/esr24 uplift.
Flags: needinfo?(jseward)
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?
Flags: needinfo?(jseward)
Comment on attachment 762404 [details] [diff] [review]
A minimal fix

[Approval Request Comment]
Same as comment 19.
Attachment #762404 - Flags: approval-mozilla-esr24?
Attachment #762404 - Flags: approval-mozilla-beta?
Attachment #762404 - Flags: approval-mozilla-b2g26?
Attachment #762404 - Flags: approval-mozilla-beta?
Attachment #762404 - Flags: approval-mozilla-beta+
Attachment #762404 - Flags: approval-mozilla-aurora?
Attachment #762404 - Flags: approval-mozilla-aurora+
Being an ARM-only sec-low, I don't think we need this on esr24, but B2G still sounds like a good idea.
Flags: needinfo?(bbajaj)
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-
Flags: needinfo?(bbajaj)
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-
Whiteboard: [adv-main27+]
Group: core-security
You need to log in before you can comment on or make changes to this bug.