1191463 - (CVE-2015-7180) Mishandling return status in ReadbackResultWriterD3D11::Run might cause memory-safety bug

Status: RESOLVED FIXED

(Core :: Graphics, defect)

Description

[q1]

ReadbackResultWriterD3D11::Run (gfx\layers\d3d11\ReadbackManagerD3D11.cpp) mishandles failure status from ID3D10Texture2D::Map. This might cause it to use a pointer and other data from uninitialized memory to create a surface, which is then used to read (and possibly write?) data from/to memory:

41:  NS_IMETHODIMP Run()
42:  {
43:    D3D10_TEXTURE2D_DESC desc;
44:    mTask->mReadbackTexture->GetDesc(&desc);
45:
46:    D3D10_MAPPED_TEXTURE2D mappedTex;
47:    // We know this map will immediately succeed, as we've already mapped this
48:    // copied data on our task thread.
49:    HRESULT hr = mTask->mReadbackTexture->Map(0, D3D10_MAP_READ, 0, &mappedTex);
50:
51:    if (FAILED(hr)) {
52:      mTask->mSink->ProcessReadback(nullptr);
53:    }
54:
55:    {
56:      RefPtr<DataSourceSurface> surf =
57:        Factory::CreateWrappingDataSourceSurface((uint8_t*)mappedTex.pData, mappedTex.RowPitch,
58:                                                 IntSize(desc.Width, desc.Height),
59:                                                 SurfaceFormat::B8G8R8X8);
60:
61:      mTask->mSink->ProcessReadback(surf);
62:
63:      MOZ_ASSERT(surf->hasOneRef());
64:    }
65:
66:    mTask->mReadbackTexture->Unmap(0);
67:
68:    return NS_OK;
69:  }

First lines 47-8 assert that Map cannot fail, but then lines 51-3 attempt to handle failure. However, they do so imcompletely (should there be a "return <some status>;" following line 52? Or has an "else" accidentally been elided from line 54?) leaving lines 57-9 using the uninitialized struct mappedTex. Despite the comments on line 47-8, probably OOM, device failure, and possibly other conditions can cause Map to fail even under the conditions under which Run runs.

Comment 1

[Jamie Nicol [:jnicol]]

Attachment: Patch v1

Bas, are you okay to review this? Attached patch just adds a early return if the map fails, and updates the comment.

Is it a problem that we do not handle the potential Map failure in ReadbackManagerD3D11::ProcessTasks()? In its case we only call Unmap() rather than read from mappedTex.

Comment 2

[Bas Schouten (:bas.schouten)]

Comment on attachment 8649250 [details] [diff] [review]
Patch v1

Review of attachment 8649250 [details] [diff] [review]:
-----------------------------------------------------------------

::: gfx/layers/d3d11/ReadbackManagerD3D11.cpp
@@ +49,5 @@
>      HRESULT hr = mTask->mReadbackTexture->Map(0, D3D10_MAP_READ, 0, &mappedTex);
>  
>      if (FAILED(hr)) {
>        mTask->mSink->ProcessReadback(nullptr);
> +      return NS_ERROR_FAILURE;

You need to return NS_OK here, the Run() function returning an error is only for very critical failures, this is potentially a situation we can deal with.

Comment 3

[Jamie Nicol [:jnicol]]

Attachment: Patch v2

Return NS_OK instead of NS_ERROR_FAILURE.

Comment 4

[Jamie Nicol [:jnicol]]

Comment on attachment 8649369 [details] [diff] [review]
Patch v2

I wasn't sure if this needs sec approval, so asking just in case. Sorry if it doesn't.

[Security approval request comment]
> How easily could an exploit be constructed based on the patch?

Not easily

> Do comments in the patch, the check-in comment, or tests included in the patch paint a bulls-eye on the security problem?

No

> Which older supported branches are affected by this flaw?

All

> If not all supported branches, which bug introduced the flaw?

> Do you have backports for the affected branches? If not, how different, hard to create, and risky will they be?

Patch applies cleanly to all supported branches

> How likely is this patch to cause regressions; how much testing does it need?

Very unlikely, very little

Comment 5

[Andrew McCreight [:mccr8]]

It sounds like the analysis in comment 0 is accurate, and we're dereferencing an uninitialized value, so I'm going to mark this sec-high. Thanks for the patch and the bug report.

Comment 6

[Daniel Veditz [:dveditz]]

Comment on attachment 8649369 [details] [diff] [review]
Patch v2

a=dveditz for sec-approval and older branches.

Please also check this into aurora, beta, and ESR38

Comment 7

[Ryan VanderMeulen [:RyanVM]]

https://hg.mozilla.org/integration/mozilla-inbound/rev/91fe6c306c8a
https://hg.mozilla.org/releases/mozilla-aurora/rev/9294d377bbc5
https://hg.mozilla.org/releases/mozilla-beta/rev/2a38ca9cbfad
https://hg.mozilla.org/releases/mozilla-esr38/rev/aa2ecb8673b1

Comment 8

[Ryan VanderMeulen [:RyanVM]]

https://hg.mozilla.org/mozilla-central/rev/91fe6c306c8a