1535880 - Potential read of uninitialized memory in RAPL constructor

Status: RESOLVED FIXED

(Core :: General, enhancement)

Description

[mlfbrown]

Potential read of uninitialized memory in RAPL constructor
(function at https://searchfox.org/mozilla-central/source/tools/power/rapl.cpp#544)
Marking security to be on safe side

1. Declared uint32_t type;

2. type is passed to ReadValueFromPowerFile for initialization:
   ReadValueFromPowerFile("type", "", "", "%u", &type);

3. In ReadValueFromPowerFile fails without initializing type if it cannot open a file:
   (function here https://searchfox.org/mozilla-central/source/tools/power/rapl.cpp#430)
   sprintf(filename, "/sys/bus/event_source/devices/power/%s%s%s", aStr1, aStr2, aStr3);
   FILE* fp = fopen(filename, "r");
   if (!fp) return false; // No intialization of type aka aOut
   if (fscanf(fp, aScanfString, aOut) != 1) ...

4. Back in the RAPL constructor, type is stored as a field via many Domain constructors:
   mPkg = new Domain("pkg", type);
   mCores = new Domain("cores", type);
   mGpu = new Domain("gpu", type, Domain::Optional);
   mRam = new Domain("ram", type, Domain::Optional);

Comment 1

[:Gijs (he/him)]

:njn/:glandium, can you help triage this? Thanks.

Comment 2

[Nicholas Nethercote [inactive]]

The diagnosis is correct that uninitialized memory could be read if the kernel-provided /sys/bus/event_source/devices/power/type file was missing (perhaps due to a kernel configuration that lacked that file).

However, this uninitialized value only gets passed to the perf_event_open() syscall, where it (presumably) would cause that syscall to fail due to an unexpected attr.type parameter.

    struct perf_event_attr attr;
    memset(&attr, 0, sizeof(attr));
    attr.type = aType;
    attr.size = uint32_t(sizeof(attr));
    attr.config = config;

    // Measure all processes/threads. The specified CPU doesn't matter.
    mFd = perf_event_open(&attr, /* aPid = */ -1, /* aCpu = */ 0,
                          /* aGroupFd = */ -1, /* aFlags = */ 0);
    if (mFd < 0) {
      Abort(
          "perf_event_open() failed\n"
          "- Did you run as root (e.g. with |sudo|) or set\n"
          "  /proc/sys/kernel/perf_event_paranoid to 0, as required?");
    }

So I don't think there's a security problem here; the program would just abort.

Furthermore, RAPL is a standalone utility program used to obtain system-wide power usage estimates from Intel CPUs. It is not part of Firefox, and it is not shipped to users.

I will write a patch that avoids possibility of reading the uninitialized memory. The only effect of this will be to abort slightly earlier with a more useful error message.

Comment 3

[Nicholas Nethercote [inactive]]

So I don't think there's a security problem here; the program would just abort.

I confirmed this by changing the code to look for a file called type2 instead of type. It aborted in the code snippet shown in comment 2.

Comment 4

[Nicholas Nethercote [inactive]]

Attachment: Bug 1535880 - Always check ReadValueFromPowerFile() return value. r=glandium

This makes RAPL abort with more informative error messages if certain
kernel-provided files aren't present.

Comment 5

[:Gijs (he/him)]

Marking this sec-other because it doesn't affect Firefox, and core-sec-release group for the same reason.

Comment 6

[Nicholas Nethercote [inactive]]

I don't think this need to be a security bug at all. As comment 2 says, the outcome of the uninitialized read is benign.

Comment 7

[:Gijs (he/him)]

(In reply to Nicholas Nethercote [:njn] from comment #6)

I don't think this need to be a security bug at all. As comment 2 says, the outcome of the uninitialized read is benign.

wfm.

Comment 8

[Pulsebot]

Pushed by [e-mail]:
https://hg.mozilla.org/mozilla-central/rev/300d3075ae04
Always check ReadValueFromPowerFile() return value. r=glandium

Comment 9

[Oana Pop-Rus]

https://hg.mozilla.org/mozilla-central/rev/300d3075ae04