Closed Bug 807096 Opened 7 years ago Closed 6 years ago

Texture memory exposure and abort() with sketch3d.com

Categories

(Core :: Canvas: WebGL, defect)

x86
macOS
defect
Not set

Tracking

()

RESOLVED WORKSFORME
Tracking Status
firefox22 --- wontfix
firefox23 - wontfix
firefox24 - wontfix
firefox25 --- wontfix
firefox-esr17 - wontfix
firefox-esr24 --- wontfix
b2g18 --- wontfix
b2g18-v1.0.0 --- wontfix
b2g18-v1.0.1 --- wontfix
b2g-v1.1hd --- wontfix

People

(Reporter: jrmuizel, Assigned: milan)

Details

(Keywords: sec-high)

Attachments

(5 files, 5 obsolete files)

2.62 KB, patch
milan
: review+
abillings
: sec-approval+
Details | Diff | Splinter Review
3.29 KB, patch
milan
: review+
Details | Diff | Splinter Review
3.24 KB, patch
milan
: review+
Details | Diff | Splinter Review
3.19 KB, patch
milan
: review+
Details | Diff | Splinter Review
189 bytes, text/plain
Details
If I force Intel GPU on OS X 10.7 and go to the following url:
https://sketchfab.com/show/hZimZzGxleN5z9GgguH5DgvbQDl

I get a crash in glGenerateMipMap with a 4096x4096 texture.

This also causes Chrome's gpu process to crash.
Critsmash Triag: Can we get more information or a dump of the crash? Can you propose a security rating?
Could this be related to bug 807741?

Marking sec-high because driver crashes sound bad.
Keywords: sec-high
Jeff should we get someone on this?
For what it's worth, this doesn't crash for me on 10.8 (forcing Intel HD 4000) on 17.0.1 or nightly.
Can somebody CC me on (In reply to Andrew McCreight [:mccr8] from comment #2)
> Could this be related to bug 807741?
> 
> Marking sec-high because driver crashes sound bad.

Can somebody with access CC me on 807741 that is possibly related to this?
(In reply to Milan Sreckovic [:milan] from comment #5)
> 
> Can somebody with access CC me on 807741 that is possibly related to this?

Done.
Jeff, when you get to it, could you try to reproduce to see if it still happens, or if it was taken care of by the blacklisting?  If you still have 10.7.
Flags: needinfo?(jmuizelaar)
Assignee: nobody → jmuizelaar
In bug 737182 we limited the max texture size to 4k on Mac/Intel.

Should we just further limit it to 2k?
Assignee: jmuizelaar → bjacob
Flags: needinfo?(jmuizelaar)
(In reply to Benoit Jacob [:bjacob] from comment #8)
> In bug 737182 we limited the max texture size to 4k on Mac/Intel.
> 
> Should we just further limit it to 2k?

Are we sure this is because of the size?  If so, I agree, let's lower the limit.
I don't really know; but since Jeff can reproduce, we could verify ourselves.
Jeff, can you still reproduce?
Flags: needinfo?(jmuizelaar)
Yes.
Flags: needinfo?(jmuizelaar)
To summarize.  This crash does not happen on 10.8.  It does happen on 10.7.  It does not crash on 10.7 if the Intel/Mac limit is lowered from 4096 to 2048, but the texturing is gone from the resulting 3d surface - the shape is there, but it's all black. On 10.8, it looks correct even with the limit at 2k.
No, an ANGLE bug couldn't do that on Mac. (It could only on Windows, where ANGLE provides the entire GL implementation; on Mac it only provides the shader compiler).
(In reply to Milan Sreckovic [:milan] from comment #13)
> To summarize.  This crash does not happen on 10.8.  It does happen on 10.7. 
> It does not crash on 10.7 if the Intel/Mac limit is lowered from 4096 to
> 2048, but the texturing is gone from the resulting 3d surface - the shape is
> there, but it's all black. On 10.8, it looks correct even with the limit at
> 2k.

OK, so let's limit the max texture size on Mac+Intel to 2K. Patch coming.
Attachment #739734 - Flags: review?(jmuizelaar) → review+
Can we get this reviewed and checked in, if good?
Flags: needinfo?(jmuizelaar)
My bad, this *is* reviewed. Can we get this checked in?
Flags: needinfo?(jmuizelaar)
The patch has been reviewed - can it be checked in?
Flags: needinfo?(bjacob)
This got out of date, rebased with the latest, carrying over r=jmuizelaar.  When landing, this is bjacob's patch.
Attachment #739734 - Attachment is obsolete: true
Attachment #767363 - Flags: review+
Modification to the original patch, to only restrict the texture size on Mac+Intel on < 10.8.  On my 10.8 system, I do not run into this problem with Intel (or discrete) graphics.  This way we'd have fewer restrictions.
Attachment #767369 - Flags: review?(bjacob)
Flags: needinfo?(bjacob)
Comment on attachment 767369 [details] [diff] [review]
Limit maximum texture size to 2048 on Mac+Intel < 10.8

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

::: gfx/gl/GLContext.cpp
@@ +603,5 @@
> +                OSErr err2 = ::Gestalt(gestaltSystemVersionMinor, &minor);
> +
> +                // For 2D textures, see bug 737182 for the original restriction to 4K and
> +                // see bug 807096 for why we further restricted it to 2K on < 10.8
> +                // for good measure, we align renderbuffers on what we do for 2D textures

Nitpicky nit: in multi-line, multi-sentence comments, punctuation/case becomes important. "for good measure..." is now the start of a new sentence.
Attachment #767369 - Flags: review?(bjacob) → review+
Carrying over r=bjacob addressing a nit.
Try run here https://tbpl.mozilla.org/?tree=Try&rev=8e661b60a4af
Attachment #767363 - Attachment is obsolete: true
Attachment #767369 - Attachment is obsolete: true
Attachment #767876 - Flags: review+
Comment on attachment 767876 [details] [diff] [review]
Limit maximum texture size to 2048 on Mac+Intel < 10.8

[Security approval request comment]
How easily could an exploit be constructed based on the patch?
One could trigger the problem easily based on the fix, with the large texture.  However, there is no help as to how to exploit it.

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?
n/a

Do you have backports for the affected branches? If not, how different, hard to create, and risky will they be?
Backports would need to be created, but they are trivial, just accounting for the recent changes.

How likely is this patch to cause regressions; how much testing does it need?
There may be performance regressions because some of the larger textures are now handled in software.
Attachment #767876 - Flags: sec-approval?
Comment on attachment 767876 [details] [diff] [review]
Limit maximum texture size to 2048 on Mac+Intel < 10.8

sec-approval+ for trunk. Please also prepare and nominate patches for aurora and beta once this has landed on trunk.
Attachment #767876 - Flags: sec-approval? → sec-approval+
Checkin-needed - for trunk, with the attached patch.  I will prepare the aurora and beta versions.
Keywords: checkin-needed
Attached patch Aurora matching patch (obsolete) — Splinter Review
Aurora version of the patch.  Only picking up the Intel check.
Attachment #768511 - Flags: review?(bjacob)
Attached patch Beta matching patch (obsolete) — Splinter Review
Beta matching patch. Only picking up the Intel check.
Attachment #768513 - Flags: review?(bjacob)
Attachment #768511 - Flags: review?(bjacob) → review+
Attachment #768513 - Flags: review?(bjacob) → review+
Attachment #768511 - Attachment is obsolete: true
Attachment #768513 - Attachment is obsolete: true
Attachment #768511 - Attachment is obsolete: false
Attachment #768513 - Attachment is obsolete: false
Proper Aurora matching patch, carrying over r+ from bjacob.
Attachment #768511 - Attachment is obsolete: true
Attachment #768566 - Flags: review+
Proper Beta patch, carrying over r+ from bjacob.
Attachment #768513 - Attachment is obsolete: true
Attachment #768567 - Flags: review+
Comment on attachment 768566 [details] [diff] [review]
Aurora matching patch

[Security approval request comment]

See the m-c comment.  The same applies here.
Attachment #768566 - Flags: sec-approval?
Comment on attachment 768567 [details] [diff] [review]
Beta matching patch

[Security approval request comment]

See the m-c comment.  The same applies here.
Attachment #768567 - Flags: sec-approval?
Comment on attachment 768566 [details] [diff] [review]
Aurora matching patch

sec-approval? is only for trunk. You need to ask for approval-mozilla-aurora and beta and fill out the form for those.
Attachment #768566 - Flags: sec-approval?
Attachment #768567 - Flags: sec-approval?
Comment on attachment 768566 [details] [diff] [review]
Aurora matching patch

[Approval Request Comment]
Bug caused by (feature/regressing bug #): n/a
User impact if declined: sec-high in the wild
Testing completed (on m-c, etc.): yes
Risk to taking this patch (and alternatives if risky): low
String or IDL/UUID changes made by this patch: n/a
Attachment #768566 - Flags: approval-mozilla-aurora?
Comment on attachment 768567 [details] [diff] [review]
Beta matching patch

[Approval Request Comment]
Bug caused by (feature/regressing bug #): n/a
User impact if declined: sec-high in the wild
Testing completed (on m-c, etc.): yes
Risk to taking this patch (and alternatives if risky): low
String or IDL/UUID changes made by this patch: n/a
Attachment #768567 - Flags: approval-mozilla-beta?
Comment on attachment 768566 [details] [diff] [review]
Aurora matching patch

Low risk sec-high, approving for branches
Attachment #768566 - Flags: approval-mozilla-aurora? → approval-mozilla-aurora+
Attachment #768567 - Flags: approval-mozilla-beta? → approval-mozilla-beta+
Do we need this on ESR17 or B2G18?
(In reply to Alex Keybl [:akeybl] from comment #38)
> Do we need this on ESR17 or B2G18?

This is OS X only code change, so we don't need it for B2G18.  I'll attach the patch for ESR17 - do we uplift sec-high as a matter of course? I'll ask for approval, so that we can decide.

Checkin-needed for central, aurora and beta for now.
[Approval Request Comment]
If this is not a sec:{high,crit} bug, please state case for ESR consideration:
User impact if declined: sec-high
Fix Landed on Version: 23 (while in beta)
Risk to taking this patch (and alternatives if risky): low
String or UUID changes made by this patch: n/a

See https://wiki.mozilla.org/Release_Management/ESR_Landing_Process for more info.
Attachment #769414 - Flags: review+
Attachment #769414 - Flags: approval-mozilla-esr17?
Attachment #769414 - Flags: approval-mozilla-b2g18-
https://hg.mozilla.org/mozilla-central/rev/821104668b01
Assignee: bjacob → milan
Status: NEW → RESOLVED
Closed: 7 years ago
Resolution: --- → FIXED
Target Milestone: --- → mozilla25
Just talked to Milan and Benoit about this. The patch causes my nightly to render completely white if the window is larger than 2048px, which it typically is.
Matt, can you please test to verify this is fixed?
Keywords: verifyme
QA Contact: mwobensmith
I can no longer reproduce this problem with the limit set back to 4096. I suggest we back it all out to avoid causing bug 891568.
Backed out:  https://hg.mozilla.org/integration/mozilla-inbound/rev/e35388d6d558

We should back this out of mozilla-aurora and mozilla-beta too. Milan, I'll leave that to you.
Status: RESOLVED → REOPENED
Resolution: FIXED → ---
Backing out the change.
Attachment #774261 - Flags: sec-approval?
Attachment #774261 - Flags: approval-mozilla-beta?
Attachment #774261 - Flags: approval-mozilla-aurora?
Attachment #769414 - Flags: approval-mozilla-esr17?
Comment on attachment 774261 [details]
Request to back out the changes on uplift branches.

>The original problem is no longer reproducible even without the fix, and the fix caused bug 891568.  It has been backed out on inbound, would like this backed out of beta and aurora as well.
Comment on attachment 774261 [details]
Request to back out the changes on uplift branches.

Well, you already backed it out so you don't need sec-approval now... 

Really, though, the main purpose of sec-approval is to avoid accidental 0 days and exposure. Since this is already in the trees, I'm less concerned (yes, I recognize that any change can draw attention).

I expect that release management will approve the backout unless it is dangerous somehow. I assume it is not?
Attachment #774261 - Flags: sec-approval?
The backout on alpha/beta gets us back to where we were a few days ago, and we can't reproduce the original issue, so, yes, I'd say it's safe.
Comment on attachment 774261 [details]
Request to back out the changes on uplift branches.

Giving a + for branches.
Attachment #774261 - Flags: approval-mozilla-beta?
Attachment #774261 - Flags: approval-mozilla-beta+
Attachment #774261 - Flags: approval-mozilla-aurora?
Attachment #774261 - Flags: approval-mozilla-aurora+
This never made it to b2g, did it? If not, we don't need to track it there.
(In reply to Al Billings [:abillings] from comment #56)
> This never made it to b2g, did it? If not, we don't need to track it there.

Right - it never made it to B2G.
(In reply to Ryan VanderMeulen [:RyanVM UTC-4] from comment #43)
> https://hg.mozilla.org/releases/mozilla-aurora/rev/c921b322bd98
> https://hg.mozilla.org/releases/mozilla-beta/rev/7dcf4ff8fb59

With the approval given in comment 55, tagging checkin-needed for the backout on these two branches.
Keywords: checkin-needed
(In reply to Jeff Muizelaar [:jrmuizel] from comment #47)
> I can no longer reproduce this problem with the limit set back to 4096. I
> suggest we back it all out to avoid causing bug 891568.

Now that we've done the backout, marking "works for me"
Status: REOPENED → RESOLVED
Closed: 7 years ago6 years ago
Resolution: --- → WORKSFORME
Comment on attachment 768566 [details] [diff] [review]
Aurora matching patch

Per comment 49, this isn't landing on Aurora at this point.
Attachment #768566 - Flags: approval-mozilla-aurora+
Comment on attachment 768567 [details] [diff] [review]
Beta matching patch

Per comment 49, this isn't landing on Beta at this point.
Attachment #768567 - Flags: approval-mozilla-beta+
Group: core-security → core-security-release
Group: core-security-release
You need to log in before you can comment on or make changes to this bug.