1751964 - Crash in [@ DdQueryDisplaySettingsUniqueness]

Status: VERIFIED FIXED

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

Description

[Chris Martin [:cmartin]]

Stack trace here

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 amdh264enc64.dll <unknown in amdh264enc64.dll> 
7 amdh264enc64.dll <unknown in amdh264enc64.dll> 
8 amdh264enc64.dll <unknown in amdh264enc64.dll> 
9 amdh264enc64.dll <unknown in amdh264enc64.dll> 

Another

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 mfx_mft_h264ve_64.dll <unknown in mfx_mft_h264ve_64.dll> 
7 mfx_mft_h264ve_64.dll <unknown in mfx_mft_h264ve_64.dll> 
8 mfx_mft_h264ve_64.dll <unknown in mfx_mft_h264ve_64.dll> 
9 mfx_mft_h264ve_64.dll <unknown in mfx_mft_h264ve_64.dll> 

Comment 1

[Chris Martin [:cmartin]]

Gian-Carlo thinks that this may be WebRTC performing hardware encoding in content process.

With Win32k Lockdown, that won't be possible. Is it alright to disable HW encoding for WebRTC?

NI-ing :padenot, who said he would be able to get the right people involved in this discussion.

Comment 2

[Paul Adenot (:padenot)]

John, where we stand these days w.r.t. h264? Is it expected that we're touching a hardware encoder for h264 in thread WebrtcWorker #4? What the best route to not do that?

Comment 3

[Gian-Carlo Pascutto [:gcp]]

Is it expected that we're touching a hardware encoder for h264 in thread WebrtcWorker #4

To be clear: the main problem for us is that this is in Process Type: content which isn't supposed to have any access to the Windows graphics stack.

Comment 4

[Gian-Carlo Pascutto [:gcp]]

Guessing, maybe it's this: https://searchfox.org/mozilla-central/source/dom/media/webrtc/libwebrtcglue/TaskQueueWrapper.h#133
created through this: https://searchfox.org/mozilla-central/source/third_party/libwebrtc/video/video_stream_encoder.cc#341
which then creates a platform encoder here: https://searchfox.org/mozilla-central/source/dom/media/webrtc/libwebrtcglue/WebrtcMediaDataEncoderCodec.cpp#57

Comment 5

[Gian-Carlo Pascutto [:gcp]]

If my tracing is correct, we should guard https://searchfox.org/mozilla-central/source/dom/media/webrtc/libwebrtcglue/WebrtcVideoCodecFactory.cpp#98 with a check for win32k lockdown status, and refuse to use hardware accelerated video encoding if we're in a locked down process.

Checking, we'd fall back to OpenH264 encoding on Windows, but not on Linux. That's fine, because Linux doesn't have win32k lockdown.

Comment 6

[Bobby Holley (:bholley)]

Seems like the right long-term fix is to move the WMF encoding into the UtilityWithWin32k process. Short-term we might be able to just use software encoding to get win32k out the door. Would like to hear from the WebRTC folks on how much of a user impact that would have.

Comment 7

[Jim Mathies [:jimm]]

Odd, both the gpu and rdd processes are up and running, and I would expect hardware encoding to take place in one of those. I've reached out to John to have a look.

Comment 8

[John Lin [:jhlin][:jolin]]

Sorry for missing the ni.

I will add the guard Gian-Carlo mentioned in comment 5 as a short term fix and eventually move the WMF calls out of content process. With the former, WebRTC will fallback to OpenH264 and probably use software WMF encoder after bug 1741244 is landed, and re-enable the hardware encoder after it's moved.

Comment 9

[Jim Mathies [:jimm]]

I do not think we should ship win32k locdown to 100% of release users until the remoting work is complete.

We do not have telemetry on hardware encoding usage currently. But to give a sense of the number of Windows users who might be impacted, about 40% of release users have H264 hardware decoding support. This is a high water estimate since the codec used can be manipulated by the services. (H264 though definitely is the more popular codec for playback.) We can assume sites like Google Meet probably default to VPX, but that's not going to be true for every conferencing service.

Comment 10

[Gian-Carlo Pascutto [:gcp]]

But to give a sense of the number of Windows users who might be impacted, about 40% of release users have H264 hardware decoding support.

You'll have to multiply that by the number of installations using WebRTC in the first place though.

As discussed, let's get Telemetry in ASAP to understand the impact, so we can make informed calls.

Comment 11

[John Lin [:jhlin][:jolin]]

Attachment: Bug 1751964 - guard HW encoder enumeration with Win32k lock down state. r?gcp

Comment 12

[Pulsebot]

Pushed by [e-mail]:
https://hg.mozilla.org/integration/autoland/rev/3a231a353405
guard HW encoder enumeration with Win32k lock down state. r=gcp,media-playback-reviewers,bryce

Comment 13

[Cristian Tuns]

https://hg.mozilla.org/mozilla-central/rev/3a231a353405

Comment 14

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

The patch landed in nightly and beta is affected.
:jhlin, is this bug important enough to require an uplift?
If not please set status_beta to wontfix.

For more information, please visit auto_nag documentation.

Comment 15

[John Lin [:jhlin][:jolin]]

Comment on attachment 9262933 [details]
Bug 1751964 - guard HW encoder enumeration with Win32k lock down state. r?gcp

Beta/Release Uplift Approval Request

• User impact if declined: Content process crash if win32k lockdown pref is on.
• Is this code covered by automated tests?: No
• Has the fix been verified in Nightly?: Yes
• Needs manual test from QE?: Yes
• If yes, steps to reproduce: 0. On a Windows machine with either Intel integrated or AMD GPU. If there is no webcam, turn on pref media.navigator.streams.fake

1. Make sure pref security.sandbox.content.win32k-disable is turned on
2. Go to https://mozilla.github.io/webrtc-landing/pc_test.html
3. Check 'Require H.264 video'
4. Click 'Start'
   Expected behavior: the tab should not crash

• List of other uplifts needed: None
• Risk to taking this patch: Low
• Why is the change risky/not risky? (and alternatives if risky): This is a very simple change that adds a guard that checks for the current win32k lockdown state.
• String changes made/needed:

Comment 16

[Pascal Chevrel:pascalc (PTO until Sept 2)]

Comment on attachment 9262933 [details]
Bug 1751964 - guard HW encoder enumeration with Win32k lock down state. r?gcp

Approved for 98 beta 5, thanks.

Comment 17

[Pascal Chevrel:pascalc (PTO until Sept 2)]

https://hg.mozilla.org/releases/mozilla-beta/rev/8a24d83869fc

Comment 18

[Hani Yacoub, Desktop QA]

Verified as fixed on Windows 10 x64 and on Windows 10 x86.