737182 - 2D texture corruption on Mac/Intel with large texture sizes >= 4993

Status: VERIFIED FIXED

(Core :: Graphics, defect)

Description

[Hilary Hall [:hillzy]]

Attachment: Image corruption image

When I go to: http://www.dannijo.com/product_info.php?products_id=766 and click on the image to view details, check the file I attached to see what happens:

Hardware Info:
11" Macbook Air - Mid 2011
Graphics  Intel HD Graphics 3000 384 MB
Software  Mac OS X Lion 10.7.3 (11D50b)

Running Firefox 11.0

Comment 1

[Benoit Girard (:BenWa)]

STR: Click on the image

I can confirm the bug by running on my integrated gpu, not discrete:
15-inch, Early 2011
Intel Inc. -- Intel HD Graphics 3000 OpenGL Engine -- 2.1 APPLE-7.14.5

Vague regression range (between firefox 9 and 10):
http://hg.mozilla.org/releases/mozilla-release/rev/72ad46d416ce is affected
http://hg.mozilla.org/releases/mozilla-release/rev/c4405d7a95f6 is not

Comment 2

[Joe Drew (not getting mail)]

This is a really big image -

(gdb) p iwidth
$1 = 5455
(gdb) p iheight
$2 = 5455

While the Intel card apparently allows 8192, this makes me suspicious....

Comment 3

[Benoit Jacob [:bjacob] (mostly away)]

Our experience in bug 684882 may be worth keeping in mind here (intel/mac driver too optimistic with its max tex size)

Comment 4

[Joe Drew (not getting mail)]

It's not a strict image size problem; resizing the image to 4097x4097 lets it work, as does 4500x4500, but 5000x5000 doesn't.

Comment 5

[Joe Drew (not getting mail)]

Given that this is not a simple problem, I'm going to bow out of this in favour of working on higher-priority items.

Comment 6

[Joe Drew (not getting mail)]

Attachment: testcase

This is a simple save-as of the website, and reproduces the bug on load.

Comment 7

[Benoit Jacob [:bjacob] (mostly away)]

Thanks for hiding this bug. uninitialized-video-memory-exposed bugs with a reproducible testcase are definitely sg:high.

Comment 8

[Benoit Jacob [:bjacob] (mostly away)]

Here is a testcase using WebGL, showing this to be a driver bug with huge 2D textures. It uses a 5455x5455 texture. This was directly adapted from the cube map testcase from bug 684882.

http://people.mozilla.org/~bjacob/webgltexture2D5k.html

STR:
1. use gfxCardStatus to force usage of the Intel GPU
2. go to above testcase

Confirmed to reproduce in both Firefox and Chrome.

Probable solution: like in bug 684882, limit texture size on Mac/Intel. Here, 4096 seems like the natural choice, unless someone can reproduce the bug at sizes <= 4096. By that occasion, we should probably move the existing cube map workaround from WebGL implementation into GLContext as well.

Comment 9

[Benoit Jacob [:bjacob] (mostly away)]

Attachment: small testcase using WebGL

Comment 10

[Benoit Jacob [:bjacob] (mostly away)]

Attachment: near-minimal testcase using only canvas2D

Here's a more or less minimal testcase, without WebGL, which means in particular that the Mac will naturally stay on the integrated Intel GPU i.e. no need for gfxCardStatus anymore to reproduce.

public URL:
http://people.mozilla.org/~bjacob/canvas2D5k.html

STR:
1. use Mac OSX 10.6+ (so we do accelerated layers) with Intel graphics, make sure that the integrated Intel GPU is in use.
2. click above link

Confirmed to reproduce in Firefox.

Comment 11

[Benoit Jacob [:bjacob] (mostly away)]

Attachment: limit max tex size on mac/intel

This should fix it, assuming that 2D texture sizes <= 4096 are safe.

I wasn't sure what was the best way to simulate a ERROR_INVALID_VALUE. In WebGLContext we supplant the native GL error system, but that's not the case in GLContext outside of debug mode at the moment. So I thought the simplest was to change the width and height to -1. Indeed, GLsizei is signed, negative values explicitly generate INVALID_VALUE, and even if they were interpreted as a huge positive value (in case of a driver bug), that would still be INVALID_VALUE (larger than max value).

I've been wondering how safe it is to let this workaround depend on the VENDOR string, given Mac GPU switching. I think it's quite safe as the GLContext is the LayerManager's, which is created when the Window is created, which is earlier than when content can force a GPU switch (by creating a WebGL context). One possible attack though would go as follows:
 1. browser starts, main window using GL layers on Intel GPU
 2. create a WebGL context -> switch all to discrete GPU
 3. create a new window. This step probably requires some social engineering i.e. I don't think that content can create new windows entirely by itself.
 4. New window's GLContext is created on discrete GPU, hence it doesn't have the texture size restriction.
 5. drop reference to WebGL context object and canvas
 6. wait for GC to happen (about 20 seconds)
 7. When the WebGL context is GC'd, the Mac switches back to Intel GPU
 8. in the second browser window created at step 3, create a very large 2d canvas as in the testcase here.

Should we worry about such a convoluted attack path?

Comment 12

[Benoit Jacob [:bjacob] (mostly away)]

Attachment: limit max tex size on mac/intel

Let's initialize these max-size variables in the ctor to avoid any headache if the initialization code setting them is ever not called.

Comment 13

[Benoit Jacob [:bjacob] (mostly away)]

Attachment: drop WebGL cube map workaround

Once the above GLContext patch lands, we won't need the WebGL-specific tweak anymore.

Keeping this a separate patch because I want to only ask for aurora/beta approval for the GLContext patch, not for this one.

Comment 14

[Benoit Jacob [:bjacob] (mostly away)]

(In reply to Benoit Girard (:BenWa) from comment #1)
> Vague regression range (between firefox 9 and 10):
> http://hg.mozilla.org/releases/mozilla-release/rev/72ad46d416ce is affected
> http://hg.mozilla.org/releases/mozilla-release/rev/c4405d7a95f6 is not

This could be explained by the fact that we used to have a bug whereby we stayed on the discrete GPU all the time (see bug 646043) and eventually fixed it.

Comment 15

[Benoit Jacob [:bjacob] (mostly away)]

We bisected the minimum texture size reproducing this on Etienne's Mac. The smallest size reproducing this is 4993.

Comment 16

[Benoit Girard (:BenWa)]

(In reply to Benoit Jacob [:bjacob] from comment #14)
> (In reply to Benoit Girard (:BenWa) from comment #1)
> > Vague regression range (between firefox 9 and 10):
> > http://hg.mozilla.org/releases/mozilla-release/rev/72ad46d416ce is affected
> > http://hg.mozilla.org/releases/mozilla-release/rev/c4405d7a95f6 is not
> 
> This could be explained by the fact that we used to have a bug whereby we
> stayed on the discrete GPU all the time (see bug 646043) and eventually
> fixed it.

That's true, I didn't pay attention to that.

Comment 17

[Benoit Girard (:BenWa)]

Comment on attachment 607910 [details] [diff] [review]
limit max tex size on mac/intel

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

::: gfx/gl/GLContext.h
@@ +1691,5 @@
> +        // max texture size that they report. So we check ourselves against our own values
> +        // (mMax[CubeMap]TextureSize) which we tweak as needed.
> +        GLsizei maxSize = target == LOCAL_GL_TEXTURE_2D
> +                          ? mMaxTextureSize
> +                          : mMaxCubeMapTextureSize;

Why do we want to use MaxCubeMapTextureSize for non GL_TEXTURE_2D target? I know for example on mobile we want to use the external image target and I don't think it has any relationship with CubeMaps.

@@ +2399,5 @@
>          AFTER_GL_CALL;
>      }
>  
>      void fTexImage2D(GLenum target, GLint level, GLint internalformat, GLsizei width, GLsizei height, GLint border, GLenum format, GLenum type, const GLvoid *pixels) {
> +        HandleIllegalTextureSizes(target, &width, &height);

This will return w=-1,h=-1. Do we know for sure that glTexImage2D and the likes will handle this? I don't like relaying on drivers for error checking. If so I'd like to see a comment explaining why we know it's safe.

Comment 18

[Benoit Jacob [:bjacob] (mostly away)]

(In reply to Benoit Girard (:BenWa) from comment #17)
> Comment on attachment 607910 [details] [diff] [review]
> limit max tex size on mac/intel
> 
> Review of attachment 607910 [details] [diff] [review]:
> -----------------------------------------------------------------
> 
> ::: gfx/gl/GLContext.h
> @@ +1691,5 @@
> > +        // max texture size that they report. So we check ourselves against our own values
> > +        // (mMax[CubeMap]TextureSize) which we tweak as needed.
> > +        GLsizei maxSize = target == LOCAL_GL_TEXTURE_2D
> > +                          ? mMaxTextureSize
> > +                          : mMaxCubeMapTextureSize;
> 
> Why do we want to use MaxCubeMapTextureSize for non GL_TEXTURE_2D target? I
> know for example on mobile we want to use the external image target and I
> don't think it has any relationship with CubeMaps.

Oh. I didn't know of other texture targets than 2D and Cube Maps. Are you referring to some extension that we are using somewhere? Link needed.

> 
> @@ +2399,5 @@
> >          AFTER_GL_CALL;
> >      }
> >  
> >      void fTexImage2D(GLenum target, GLint level, GLint internalformat, GLsizei width, GLsizei height, GLint border, GLenum format, GLenum type, const GLvoid *pixels) {
> > +        HandleIllegalTextureSizes(target, &width, &height);
> 
> This will return w=-1,h=-1. Do we know for sure that glTexImage2D and the
> likes will handle this? I don't like relaying on drivers for error checking.
> If so I'd like to see a comment explaining why we know it's safe.

True. I was just thinking that we've never seen a driver bug whereby texImage2D would fail to generate INVALID_VALUE on -1 sizes, but OK. So, if we don't want to rely on the driver to generate INVALID_VALUE for us, we have to implement our own GL error system, like we do in the WebGL impl. The patch is going to be a lot more intrusive. Do you prefer that still?

Comment 19

[Benoit Jacob [:bjacob] (mostly away)]

Alternative approach: to further increase the likelihood that the GL impl will generate INVALID_VALUE on our texImage2D call, we could set level to -1.

Comment 20

[Benoit Girard (:BenWa)]

I was thinking of just having an early return (We could use a macro maybe). I know that we could use GL_TEXTURE_RECTANGLE, the other target isn't used for glTexImage2D but is used for others calls like glBind.

http://www.khronos.org/registry/gles/extensions/OES/OES_EGL_image_external.txt

I don't want to implement our own GL error system. If your sure we will get INVALID_VALUE everywhere just add a comment to explain that we rely on this behavior.

Comment 21

[Benoit Jacob [:bjacob] (mostly away)]

If we just early-return without somehow generating INVALID_VALUE, we break the GL spec and we'll be scratching our heads some time down the road about strange bugs ("why is this texture not correctly uploaded?").

I'd much rather keep GLContext compliant with the GL spec as much as possible.

Comment 22

[Benoit Jacob [:bjacob] (mostly away)]

I don't understand why this would be only vector-high and not high. This bug directly, by itself, allows an attacker to read random video memory.

Comment 23

[Benoit Jacob [:bjacob] (mostly away)]

Is it vector-high only because this is a bug in the Mac/Intel driver that's exposed by Firefox, not a bug in Firefox itself?

Comment 24

[Joe Drew (not getting mail)]

Comment on attachment 607910 [details] [diff] [review]
limit max tex size on mac/intel

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

I take Benoit's review comments and adopt them as my own!

Comment 25

[Jeff Muizelaar [:jrmuizel]]

Comment on attachment 607912 [details]
drop WebGL cube map workaround

Dropping review until the issues with the other patch are sorted out.

Comment 26

[Benoit Jacob [:bjacob] (mostly away)]

Attachment: limit max tex size on mac/intel

How about this approach instead. The helper becomes IsTextureSizeLegalForTarget() returning bool; it checks for cube map targets and assumes that anything else behaves like TEXTURE_2D, which is probably safer; and in case of an illegal size, it now also passes -1 for |level| and for |border|, maximizing chances to get an INVALID_VALUE (there are now 4 different invalid values passed to these GL functions, so the GL would have to be really wrong to fail to generate INVALID_VALUE). Also, |null| is passed for the texture data pointer so that in the worst case, we'd be derefing null instead of an arbitrary pointer.

If you don't like that, here are the other possibilities, choose one:
 1. we use a different GL function to artificially generate a INVALID_VALUE. Downside: APItraces will look very, very mysterious ("wtf does glCopyTexImage2D call glSomeUnrelatedFunc??")
 2. we implement our own GL error system cooperating with the native GL error system. There is all the code for that in the WebGL impl. Downside: more intrusive change, won't get beta approval.
 3. we fail silently without generating an error. Downside: we break the GL spec, will cause headaches.

Comment 27

[Benoit Girard (:BenWa)]

Comment on attachment 609188 [details] [diff] [review]
limit max tex size on mac/intel

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

Looks good, minot comment nit.

::: gfx/gl/GLContext.cpp
@@ +446,5 @@
> +        if (mVendor == VendorIntel) {
> +            // see bug 737182 for 2D textures, bug 684822 for cube map textures.
> +            mMaxTextureSize        = NS_MIN(mMaxTextureSize,        4096);
> +            mMaxCubeMapTextureSize = NS_MIN(mMaxCubeMapTextureSize, 512);
> +            // for good measure, we align renderbuffers on what we do for 2D textures

The grammar seems off to me. How about:
For good measure we do the same for renderbuffers.

::: gfx/gl/GLContext.h
@@ +2405,5 @@
>      }
>  
>      void fTexImage2D(GLenum target, GLint level, GLint internalformat, GLsizei width, GLsizei height, GLint border, GLenum format, GLenum type, const GLvoid *pixels) {
>          BEFORE_GL_CALL;
> +        if (IsTextureSizeLegalForTarget(target, width, height)) {

If we cared about performance we could add the check only for Intel by setting fTexImage2D to a wrapper function. But we shouldn't be calling these often.

Comment 28

[Benoit Jacob [:bjacob] (mostly away)]

Regarding the performance comment: maybe we can rename IsTextureSizeLegalForTarget to something slightly different and have it do the platform check.

I am not worried about texImage2D which is very expensive anyway, but it is indeed not great to add overhead to copyTexImage2D as a matter of principle (in practice, this is still negligible overhead).

Comment 29

[Benoit Jacob [:bjacob] (mostly away)]

Attachment: limit max tex size on mac/intel

How about this, for performance?

Comment 30

[Benoit Girard (:BenWa)]

Comment on attachment 609343 [details] [diff] [review]
limit max tex size on mac/intel

LGTM

Comment 31

[Benoit Jacob [:bjacob] (mostly away)]

http://hg.mozilla.org/integration/mozilla-inbound/rev/7474b9e08e0d

Comment 32

[Ed Morley [:emorley]]

https://hg.mozilla.org/mozilla-central/rev/7474b9e08e0d

Comment 33

[Benoit Jacob [:bjacob] (mostly away)]

Comment on attachment 609343 [details] [diff] [review]
limit max tex size on mac/intel

[Approval Request Comment]
Regression caused by (bug #): ever since we enabled GL layers and WebGL on Mac (pre-Firefox 4)
User impact if declined: video memory leakage (information exposure) vulnerability on Mac. Exploitable just by creating a 2d canvas of size 5kx5k, or an image of that size, or a WebGL texture of that size.
Testing completed (on m-c, etc.): Been on m-c for a few days
Risk to taking this patch (and alternatives if risky): seems low risk. At worst  (if we were crazy unlucky) it could expose a different driver bug.
String changes made by this patch: none

Comment 34

[Alex Keybl [:akeybl]]

Comment on attachment 609343 [details] [diff] [review]
limit max tex size on mac/intel

[Triage Comment]
We'll be on the lookout for driver regressions after this lands - approving for Aurora 13 and Beta 12.

Comment 35

[Benoit Jacob [:bjacob] (mostly away)]

http://hg.mozilla.org/releases/mozilla-aurora/rev/3674e711982a
http://hg.mozilla.org/releases/mozilla-beta/rev/f80a1c880b7a

Comment 36

[Benoit Jacob [:bjacob] (mostly away)]

http://hg.mozilla.org/integration/mozilla-inbound/rev/bd9c99b055ab

Comment 37

[Ed Morley [:emorley]]

(In reply to Benoit Jacob [:bjacob] from comment #36)
> http://hg.mozilla.org/integration/mozilla-inbound/rev/bd9c99b055ab

https://hg.mozilla.org/mozilla-central/rev/bd9c99b055ab

Comment 38

[Lukas Blakk [:lsblakk] use ?needinfo]

[Triage Comment]
Now that there's been some bake time on aurora/beta please go ahead and nominate for ESR landing as per https://wiki.mozilla.org/Release_Management/ESR_Landing_Process

Comment 39

[Benoit Jacob [:bjacob] (mostly away)]

http://hg.mozilla.org/releases/mozilla-esr10/rev/d35ae8519966

Comment 40

[Huzaifa Sidhpurwala]

Does this issue have a CVE id?

Comment 41

[Benoit Jacob [:bjacob] (mostly away)]

Notice that this bug is completely Mac-specific (seeing your redhat email address ;-) )

Comment 42

[Tracy Walker [:tracy]]

Verified with Fx 12 beta6

Comment 43

[Benoit Jacob [:bjacob] (mostly away)]

Can we unhide this bug now? It's fixed in Release and in ESR.

Comment 44

[Daniel Veditz [:dveditz]]

What's the status of the underlying driver bug? Will we be exposing users of other applications?

Comment 45

[Benoit Jacob [:bjacob] (mostly away)]

See my reply in bug 684882 comment 92 as this is very similar.

Yes, other applications are affected, all the more since this bug is about 2D textures which are used by all the applications that use OpenGL to accelerate any kind of graphics. However, this has been known publicly for a while as a google search suggests:

https://www.google.ca/search?q=mac+texture+corruption

The discussions returned by this search don't have as specific information as we discussed in this bug though, but I don't think that our conclusion here, "2D texture uploads are garbage above a certain critical size that is roughly 5000", is particularly hard to arrive to.

Comment 46

[u279076]

Tracy, can you please take a moment to verify this fixed with Firefox 14 and the latest ESR nightly? Thanks.