Closed Bug 1698732 Opened 2 months ago Closed 15 days ago

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

Categories

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

All
Windows
defect

Tracking

()

RESOLVED FIXED
90 Branch
Tracking Status
firefox-esr78 --- unaffected
firefox86 --- unaffected
firefox87 --- unaffected
firefox88 --- disabled
firefox89 --- disabled
firefox90 --- fixed

People

(Reporter: aryx, Assigned: cmartin)

References

(Blocks 1 open bug)

Details

(Keywords: crash)

Crash Data

Attachments

(3 files)

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?

Flags: needinfo?(cmartin)
Severity: -- → S4
Priority: -- → P1

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?

Flags: needinfo?(cmartin) → needinfo?(jmuizelaar)

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

Flags: needinfo?(jmuizelaar)

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

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

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?

Flags: needinfo?(Tobias.Marty)

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?

Flags: needinfo?(Tobias.Marty)

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

Assignee: nobody → 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.

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.

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.

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

Pushed by cmartin@mozilla.com:
https://hg.mozilla.org/integration/autoland/rev/e201e698220e
Change reftest-content to get Azure info without Win32k APIs r=bas,emilio
Pushed by cmartin@mozilla.com:
https://hg.mozilla.org/integration/autoland/rev/241749e6ef6d
Remove all uses of GfxInfo::GetInfo() for Azure Backend r=bas
Pushed by cmartin@mozilla.com:
https://hg.mozilla.org/integration/autoland/rev/ee1cda6fcd58
Remove Win32k usage from GfxInfo in content process r=bas
Blocks: 1698944
Blocks: 1698948
Status: NEW → RESOLVED
Closed: 15 days ago
Resolution: --- → FIXED
Target Milestone: --- → 90 Branch

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

Regressions: 1708545
You need to log in before you can comment on or make changes to this bug.