1698732 - Permanent content crashes in [@ DdQueryDisplaySettingsUniqueness], [@ GdiEntry13] with win32k lockdown enabled

Status: RESOLVED FIXED

(Core :: Security: Process Sandboxing, defect, P1)

Description

[Sebastian Hengst [:aryx] (needinfo me if it's about an intermittent or backout)]

Firefox 88.0a1 20210315214853 on Windows 8.1 (crash reports from other users also on Windows 10 and 7)

Bug 1697865 added the option to enable win32k lockdown for tabs to Settings > Nightly Experiments.

It's not possible to load any web content or about:newtab with this enabled on a Windows 8.1 machine (bp-dc8bdb22-8027-453f-a4ae-b6b130210316) - [@ GdiEntry13]

For Windows 10, we get reports like bp-5de4cbab-822e-4fa3-affa-922db0210316 [@ DdQueryDisplaySettingsUniqueness]

Crash report: https://crash-stats.mozilla.org/report/index/5de4cbab-822e-4fa3-affa-922db0210316

Reason: EXCEPTION_ACCESS_VIOLATION_READ

Top 10 frames of crashing thread:

0 gdi32.dll DdQueryDisplaySettingsUniqueness 
1 dxgi.dll long CDXGIFactory::SampleAdapters 
2 dxgi.dll long CDXGIFactory::Initialize 
3 dxgi.dll long CreateDXGIFactoryImpl 
4 dxgi.dll long CreateDXGIFactoryActualImpl 
5 dxgi.dll CreateDXGIFactory 
6 xul.dll mozilla::widget::GfxInfo::Init widget/windows/GfxInfo.cpp:764
7 xul.dll mozilla::xpcom::CreateInstanceImpl xpcom/components/StaticComponents.cpp:9460
8 xul.dll nsComponentManagerImpl::GetService xpcom/components/nsComponentManager.cpp:1368
9 xul.dll nsCOMPtr_base::assign_from_helper xpcom/base/nsCOMPtr.cpp:109

Chris, are these permanent failures expected?

Comment 1

[Bob Owen (:bobowen)]

Looks like this is from here:
https://searchfox.org/mozilla-central/rev/526a5089c61db85d4d43eb0e46edaf1f632e853a/gfx/thebes/gfxPlatform.cpp#894

It happens on one of my laptops, which I can't have tried win32k lockdown on, for some bizarre reason.

jrmuizel - do we need this in the content process?

Comment 2

[Jeff Muizelaar [:jrmuizel]]

No, we should be able to get the info from the parent process.

Comment 3

[Gian-Carlo Pascutto [:gcp]]

From digging the crash reports and the code, we're going to hit this on all dual-GPU machines (mostly pretty beefy laptops).

Comment 4

[TMart]

On my machine (single GPU desktop) this crashes the WidevineCdm plugin on Spotify.

Comment 5

[Gian-Carlo Pascutto [:gcp]]

On my machine (single GPU desktop) this crashes the WidevineCdm plugin on Spotify.

Do you have a crash report? It shouldn't even be in this process to begin with?! Is it the same crash signature?

Comment 6

[TMart]

Report ID
bp-4ca6832d-2a71-4dc3-a549-089720210316
bp-c5ea6889-068f-4532-9111-9fa3d0210316
bp-a0374a44-4a26-4479-b50a-5c1c60210316
bp-bfdf5b34-7133-408a-96b9-de2840210316
all link to this crash signature. I get the yellow bar on the Spotify web player that says WidevineCdm crashed, do you want to submit a crash report?

Comment 7

[Gian-Carlo Pascutto [:gcp]]

Thanks! That's a different bug, but a really interesting one. I'll file a new bug.

Comment 8

[Chris Martin [:cmartin]]

It looks like we may be able to get away with not using GfxInfo::Init() at all in the content process, since it looks like all of the data members of GfxInfo are either completely unused, or they're used incidentally as a result of something calling GfxInfo::GetInfo() when it only needs a small piece of the information that doesn't rely on Win32k APIs.

After a conversation with :gcp this morning, he was concerned that it may affect crash reporting, but after talking to :gsvelto it looks like we don't need to do any crash annotations in content process, since it will just inherit the annotations from main process.

There was also concerns that some of the APIs that query this information might already be silently failing, even with the current sandbox. Luckily that doesn't seem to be the case on my own machine, but it's definitely possible that they do fail on content in some scenarios and then overwrite good information from parent, since the codepath unconditionally adds the annotations even if there is an error and/or they are empty.

Comment 9

[Chris Martin [:cmartin]]

Attachment: Bug 1698732 - Change reftest-content to get Azure info without Win32k APIs

Currently, reftest-content uses GfxInfo::GetInfo() to obtain information about
the Azure backend. GetInfo() uses Win32k APIs, and therefore will mostly
return garbage in content processes.

This adds a new way to obtain the same information directly from GfxInfo
without using Win32k APIs.

Comment 10

[Chris Martin [:cmartin]]

Attachment: Bug 1698732 - Remove all uses of GfxInfo::GetInfo() for Azure Backend

There is other JS code running in content process that uses GfxInfo::GetInfo()
when it needs to know about Azure backend. Let's just change all JS code
that reads these properties to read them directly from GfxInfo.

Comment 11

[Chris Martin [:cmartin]]

Attachment: Bug 1698732 - Remove Win32k usage from GfxInfo in content process

Make it so that GfxInfo::Init() no longer does anything and add some asserts
to several GfxInfo APIs to ensure that they are not called by accident from
content process, as calling them now will return invalid data.

Minor cleanup - While I'm at it, might as well make GfxInfo.h and
GfxInfo.cpp line up a bit better with the coding standards

Comment 12

[Pulsebot]

Pushed by [e-mail]:
https://hg.mozilla.org/integration/autoland/rev/e201e698220e
Change reftest-content to get Azure info without Win32k APIs r=bas,emilio

Comment 13

[Pulsebot]

Pushed by [e-mail]:
https://hg.mozilla.org/integration/autoland/rev/241749e6ef6d
Remove all uses of GfxInfo::GetInfo() for Azure Backend r=bas

Comment 14

[Pulsebot]

Pushed by [e-mail]:
https://hg.mozilla.org/integration/autoland/rev/ee1cda6fcd58
Remove Win32k usage from GfxInfo in content process r=bas

Comment 15

[Atila Butkovits]

https://hg.mozilla.org/mozilla-central/rev/e201e698220e

Comment 16

[BugBot [:suhaib / :marco/ :calixte]]

Since the status are different for nightly and release, what's the status for beta?
For more information, please visit auto_nag documentation.

Comment 17

[Atila Butkovits]

https://hg.mozilla.org/mozilla-central/rev/241749e6ef6d
https://hg.mozilla.org/mozilla-central/rev/ee1cda6fcd58