1774178 - Add a pref to disable Spectre mitigations for Fission web content processes

Status: RESOLVED FIXED

(Core :: JavaScript Engine: JIT, task, P3)

Description

[Jan de Mooij [:jandem]]

Additional work is needed for file:// origins, so we want to have a pref that only covers non-file Fission processes.

Comment 1

[Jan de Mooij [:jandem]]

Nika, do you know if there's a way to check for file:// processes? Ideally when we initialize the JS engine, we'd like to check something like:

if (FissionAutostart() && XRE_IsContentProcess() && !IsFileProcess()) {
  // ... Maybe override spectre pref values ... 
}

Comment 2

[Nika Layzell [:nika] (ooo, ni? for response)]

Unfortunately while we know in the parent process quite early whether the process is a file content process, as we use it to configure the process sandbox, we don't appear to send that information to the content process in a cross-platform way until much after the JS engine is initialized, in RecvRemoteType (https://searchfox.org/mozilla-central/rev/287583a4a605eee8cd2d41381ffaea7a93d7b987/dom/ipc/ContentChild.cpp#2613-2614), where we send down a FILE_REMOTE_TYPE type if its a file content process.

Sending down the full initial remote type unfortunately may be a bit of a footgun, as most content processes are created as a preallocated content process, and then specialized to a different type. The only exceptions to this right now are the extensions process (which we'd like to make able to use preallocated processes in the future), and the file process (which has different sandboxing so it needs to be started directly). Because of this it may be best to only communicate whether or not the content process is a file content process on the command line, to not mislead people into thinking they can rely on any other remote types early during content startup.

Sending down this extra information on the command line fortunately shouldn't be too difficult, though it'd require a touch of extra plumbing. The easiest way to do this is probably to add an extra boolean -isFileContent flag to GeckoArgs.h, and add it to the command line in ContentParent::BeginSubprocessLaunch, with the value mRemoteType == FILE_REMOTE_TYPE. Perhaps add a comment there as well, linking back to this bug for context as to why it's done as a boolean flag.

The flag could then be read in ContentProcess::Init, where it could be made available to the JS engine as it is starting in a number of ways. The easiest is probably to add an extra boolean flag to ContentChild, and pass it down into mContent.Init, where it would be set there. The check could then look like:

if (ContentChild* child = ContentChild::GetSingleton();
    child && !child->GetIsFileContent() && FissionAutostart()) {

That being said, I'm a bit worried about using FissionAutostart() as the decision maker here. Even with Fission enabled, we're intending to use a hybrid approach on Android where we'll load some pages in a shared content process, and identify high-value sites which have been authenticated with to load in special isolated content processes. We may want to keep the mitigations around in these shared content processes, and only disable them in the special isolated cases. In fact, there's another pref (fission.webContentIsolationStrategy, used for the android behaviour) which can be set to different values while to control how we allocate pages to processes with fission enabled, which can even be configured to use e10s-like behaviour with fission.autostart enabled. Unfortunately as I mentioned earlier, we don't know what remote type is going to be used ahead of time when the process is started, as we start pre-allocated content processes, and then specialize them later. If we needed to start having multiple sets of pre-allocated pools for web content processes which shouldn't have the mitigations disabled it would be quite a bit of complexity.

I don't suppose it's at all possible to make the decision about whether to disable JIT mitigations later after the JS engine has been started, when we're actually specializing the process? I imagine that's quite painful to do.

Comment 3

[Hsin-Yi Tsai (she/her) | away [:hsinyi]]

Tom, I believe we also need your inputs for comment 2 as well.

Comment 4

[Tom Ritter [:tjr]]

I don't think I have anything to add; from my point of view it doesn't matter if we know at time-of-process-start or time-of-first-JIT-compilation if we should enable/disable the spectre mitigations; that's a JS engine implementation detail. I can't think of a reason we would need to know before the first JIT compilation though...

Comment 5

[Jan de Mooij [:jandem]]

(In reply to Tom Ritter [:tjr] from comment #4)

I can't think of a reason we would need to know before the first JIT compilation though...

We generate trampolines for calling into C++ code early on and they can have a memory barrier after the call when mitigations are enabled. It probably makes sense for us to flip that particular pref off for desktop anyway, because it can affect performance significantly and this was always more of a theoretical issue rather than something we had PoCs for.

For the remaining mitigations: we could flip these off later, but I want to make sure this can't race with off-thread (or worker) JIT/Wasm compilation. Maybe compilations could snapshot the values at the beginning so that they're always consistent within a single compilation. I'll look into this today.

Comment 6

[Jan de Mooij [:jandem]]

Attachment: Bug 1774178 - Add a pref to disable Spectre mitigations for Fission content processes. r?nika!,tjr!,iain!

These flags are usually initialized very early on, but because child processes are
pre-allocated and then later specialized for a specific process type, we need to
reset them later. This patch adds a new API for this that has some extra assertions.

Comment 7

[Jan de Mooij [:jandem]]

Attachment: Bug 1774178 - Add test. r?nika!,iain!

Comment 8

[Jan de Mooij [:jandem]]

Attachment: Bug 1774178 - Also disable mitigations for privileged about content processes. r?nika!,tjr!

Depends on D152395

Comment 9

[Jan de Mooij [:jandem]]

@tjr: review ping for part 3?

Comment 10

[Pulsebot]

Pushed by [e-mail]:
https://hg.mozilla.org/integration/autoland/rev/7115d93b1503
Add a pref to disable Spectre mitigations for Fission content processes. r=nika,tjr,iain
https://hg.mozilla.org/integration/autoland/rev/b81752d09847
Add test. r=nika,iain
https://hg.mozilla.org/integration/autoland/rev/6a331cdec2e7
Also disable mitigations for privileged about content processes. r=nika,tjr

Comment 11

[Marian-Vasile Laza]

https://hg.mozilla.org/mozilla-central/rev/7115d93b1503
https://hg.mozilla.org/mozilla-central/rev/b81752d09847
https://hg.mozilla.org/mozilla-central/rev/6a331cdec2e7