Texture memory exposure and abort() with sketch3d.com

RESOLVED WORKSFORME

Status

()

Core
Canvas: WebGL
RESOLVED WORKSFORME
5 years ago
24 days ago

People

(Reporter: jrmuizel, Assigned: milan)

Tracking

({sec-high})

unspecified
x86
Mac OS X
sec-high
Points:
---

Firefox Tracking Flags

(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)

Details

Attachments

(5 attachments, 5 obsolete attachments)

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
(Reporter)

Description

5 years ago
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?
(Assignee)

Comment 4

5 years ago
For what it's worth, this doesn't crash for me on 10.8 (forcing Intel HD 4000) on 17.0.1 or nightly.
(Assignee)

Comment 5

5 years ago
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.
(Assignee)

Comment 7

5 years ago
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)

Updated

5 years ago
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)

Updated

5 years ago
Assignee: jmuizelaar → bjacob
Flags: needinfo?(jmuizelaar)
(Assignee)

Comment 9

5 years ago
(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.
(Assignee)

Comment 11

5 years ago
Jeff, can you still reproduce?
Flags: needinfo?(jmuizelaar)
(Reporter)

Comment 12

5 years ago
Yes.
Flags: needinfo?(jmuizelaar)
(Assignee)

Comment 13

5 years ago
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.
Is this an Angle bug?
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.
Created attachment 739734 [details] [diff] [review]
Limit max texture size to 2048 on Mac+Intel
Attachment #739734 - Flags: review?(jmuizelaar)
(Reporter)

Updated

5 years ago
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)
(Assignee)

Comment 21

5 years ago
Created attachment 767363 [details] [diff] [review]
Limit maximum texture size to 2048 on Mac+Intel

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+
(Assignee)

Comment 22

5 years ago
Created attachment 767369 [details] [diff] [review]
Limit maximum texture size to 2048 on Mac+Intel < 10.8

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+
(Assignee)

Comment 24

5 years ago
Created attachment 767876 [details] [diff] [review]
Limit maximum texture size to 2048 on Mac+Intel < 10.8

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+
(Assignee)

Comment 25

5 years ago
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+
(Assignee)

Comment 27

5 years ago
Checkin-needed - for trunk, with the attached patch.  I will prepare the aurora and beta versions.
Keywords: checkin-needed
(Assignee)

Comment 28

5 years ago
Created attachment 768511 [details] [diff] [review]
Aurora matching patch

Aurora version of the patch.  Only picking up the Intel check.
Attachment #768511 - Flags: review?(bjacob)
(Assignee)

Comment 29

5 years ago
Created attachment 768513 [details] [diff] [review]
Beta matching patch

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+
(Assignee)

Updated

5 years ago
Attachment #768511 - Attachment is obsolete: true
(Assignee)

Updated

5 years ago
Attachment #768513 - Attachment is obsolete: true
(Assignee)

Updated

5 years ago
Attachment #768511 - Attachment is obsolete: false
(Assignee)

Updated

5 years ago
Attachment #768513 - Attachment is obsolete: false
(Assignee)

Comment 30

5 years ago
Created attachment 768566 [details] [diff] [review]
Aurora matching patch

Proper Aurora matching patch, carrying over r+ from bjacob.
Attachment #768511 - Attachment is obsolete: true
Attachment #768566 - Flags: review+
(Assignee)

Comment 31

5 years ago
Created attachment 768567 [details] [diff] [review]
Beta matching patch

Proper Beta patch, carrying over r+ from bjacob.
Attachment #768513 - Attachment is obsolete: true
Attachment #768567 - Flags: review+
(Assignee)

Comment 32

5 years ago
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?
(Assignee)

Comment 33

5 years ago
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?
(Assignee)

Comment 35

5 years ago
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?
(Assignee)

Comment 36

5 years ago
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+

Updated

5 years ago
Attachment #768567 - Flags: approval-mozilla-beta? → approval-mozilla-beta+

Updated

5 years ago
status-firefox22: --- → wontfix
status-firefox23: --- → affected
status-firefox24: --- → affected
status-firefox25: --- → affected
Do we need this on ESR17 or B2G18?

Updated

5 years ago
tracking-b2g18: --- → ?
tracking-firefox-esr17: --- → ?
(Assignee)

Comment 39

4 years ago
(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.
(Assignee)

Comment 40

4 years ago
Created attachment 769414 [details] [diff] [review]
ESR17 version of the patch



[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/integration/mozilla-inbound/rev/821104668b01

We'll get the branch uplifts once this hits m-c.
Keywords: checkin-needed
https://hg.mozilla.org/mozilla-central/rev/821104668b01
Assignee: bjacob → milan
Status: NEW → RESOLVED
Last Resolved: 4 years ago
status-b2g18: --- → wontfix
status-b2g18-v1.0.0: --- → wontfix
status-b2g18-v1.0.1: --- → wontfix
status-b2g-v1.1hd: --- → wontfix
status-firefox25: affected → fixed
status-firefox-esr17: --- → affected
Resolution: --- → FIXED
Target Milestone: --- → mozilla25
https://hg.mozilla.org/releases/mozilla-aurora/rev/c921b322bd98
https://hg.mozilla.org/releases/mozilla-beta/rev/7dcf4ff8fb59
status-firefox23: affected → fixed
status-firefox24: affected → fixed
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.
(Assignee)

Comment 45

4 years ago
Opened bug 891568 for this.
Matt, can you please test to verify this is fixed?
Keywords: verifyme
QA Contact: mwobensmith
(Reporter)

Comment 47

4 years ago
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 → ---
(Assignee)

Comment 49

4 years ago
Created attachment 774261 [details]
Request to back out the changes on uplift branches.

Backing out the change.
Attachment #774261 - Flags: sec-approval?
Attachment #774261 - Flags: approval-mozilla-beta?
Attachment #774261 - Flags: approval-mozilla-aurora?
(Assignee)

Updated

4 years ago
Attachment #769414 - Flags: approval-mozilla-esr17?
(Assignee)

Comment 50

4 years ago
Backed out:  https://hg.mozilla.org/integration/mozilla-inbound/rev/e35388d6d558
(Assignee)

Comment 51

4 years ago
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?
(Assignee)

Comment 53

4 years ago
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.
https://hg.mozilla.org/mozilla-central/rev/e35388d6d558
status-firefox25: fixed → affected
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.
tracking-firefox23: --- → +
tracking-firefox24: --- → +
tracking-firefox-esr17: ? → +
(Assignee)

Comment 57

4 years ago
(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.
(Assignee)

Comment 58

4 years ago
(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
tracking-b2g18: ? → ---
https://hg.mozilla.org/releases/mozilla-aurora/rev/2c68df0e3a54
https://hg.mozilla.org/releases/mozilla-beta/rev/44bb8aa083d1
status-firefox23: fixed → affected
status-firefox24: fixed → affected
Keywords: checkin-needed
Target Milestone: mozilla25 → ---
(Assignee)

Comment 60

4 years ago
(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
Last Resolved: 4 years ago4 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+
status-firefox23: affected → wontfix
status-firefox24: affected → wontfix
status-firefox25: affected → wontfix
status-firefox-esr17: affected → wontfix
tracking-firefox23: + → -
tracking-firefox24: + → -
tracking-firefox-esr17: + → -
Keywords: verifyme
status-firefox-esr24: --- → wontfix

Updated

2 years ago
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.