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

Status: RESOLVED FIXED

(Core :: JavaScript Engine: JIT, defect)

Description

[Julian Seward [:jseward]]

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.

Comment 1

[Julian Seward [:jseward]]

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)

Comment 2

[Julian Seward [:jseward]]

This strikes me as safer:

    char buf[1024];
    memset(buf, 0, sizeof(buf));
    fread(buf, sizeof(char), sizeof(buf)-1, fp);

Comment 3

[Jan de Mooij [:jandem]]

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)?

Comment 4

[Julian Seward [:jseward]]

Well, can we get a /proc/cpuinfo dump for HTC Wildfire from anyone?

Comment 5

[(not currently active) Ted Mielczarek]

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.)

Comment 6

[Jan de Mooij [:jandem]]

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..

Comment 7

[Daniel Veditz [:dveditz]]

I'm assuming b2g18 is "unaffected" because ionmonkey is disabled, but the code does exist. Is this a feature that is used regardless?

Comment 8

[Andrew McCreight [:mccr8]]

Julian, how much of a security issue could this be?  I'm not sure what hwcaps is.

Comment 9

[Julian Seward [: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).

Comment 10

[Al Billings [:abillings - ex-MoCo]]

Who can we assign this to?

Comment 11

[Julian Seward [:jseward]]

Attachment: A minimal fix

Minimal fix, which at least (1) ensures that we can't overrun
the stack allocated buffer, and (2) stops Valgrind complaining.

Comment 12

[Jan de Mooij [:jandem]]

Julian, can you land your fix? Thanks!

Comment 13

[Jan de Mooij [:jandem]]

Marty, since Julian is not responding, can you make sure this gets fixed? Thanks!

Comment 14

[Julian Seward [:jseward]]

I'll land it; sorry for the delay.  Currently waiting for it to look ok on try.

Comment 15

[Julian Seward [:jseward]]

https://hg.mozilla.org/integration/mozilla-inbound/rev/97feecba44a4

Comment 16

[Ryan VanderMeulen [:RyanVM]]

https://hg.mozilla.org/mozilla-central/rev/97feecba44a4

Can we land the test too please?

Comment 17

[Julian Seward [:jseward]]

(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.

Comment 18

[Ryan VanderMeulen [:RyanVM]]

Please nominate this for Aurora/Beta/b2g26/esr24 uplift.

Comment 19

[Julian Seward [: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

Comment 20

[Julian Seward [:jseward]]

Comment on attachment 762404 [details] [diff] [review]
A minimal fix

[Approval Request Comment]
Same as comment 19.

Comment 21

[Ryan VanderMeulen [:RyanVM]]

https://hg.mozilla.org/releases/mozilla-aurora/rev/8634172315d5
https://hg.mozilla.org/releases/mozilla-beta/rev/80b8cfdcde61

Comment 22

[Ryan VanderMeulen [:RyanVM]]

Being an ARM-only sec-low, I don't think we need this on esr24, but B2G still sounds like a good idea.

Comment 23

[Al Billings [:abillings - ex-MoCo]]

Sec-low bugs are not normally backported to ESR branches without a specific reason.

Comment 24

[Lukas Blakk [:lsblakk] use ?needinfo]

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

Comment 25

[Ryan VanderMeulen [:RyanVM]]

FWIW, the b2g26 backport is green on Try.

Comment 26

[Preeti Raghunath(:Preeti)]

Comment on attachment 762404 [details] [diff] [review]
A minimal fix

Minus on approval‑mozilla‑b2g26 as it is a low sec issue