Closed Bug 785734 (CVE-2012-5833) Opened 12 years ago Closed 12 years ago

Mesa crashes on certain texImage2D calls involving level>0

Categories

(Core :: Graphics: CanvasWebGL, defect)

x86
Linux
defect
Not set
normal

Tracking

()

VERIFIED FIXED
mozilla19
Tracking Status
firefox15 --- wontfix
firefox16 --- affected
firefox17 + verified
firefox18 + verified
firefox19 --- verified
firefox-esr10 17+ verified
firefox-esr17 --- verified

People

(Reporter: miaubiz, Assigned: bjacob)

References

Details

(Keywords: sec-critical, sec-vector, Whiteboard: [asan][adv-track-main17+][adv-track-esr17+][qa-] webgl-test-needed)

Attachments

(9 files, 3 obsolete files)

350 bytes, text/plain
Details
902 bytes, text/plain
Details
3.65 KB, text/plain
Details
808 bytes, text/plain
Details
327 bytes, text/html
Details
864 bytes, text/html
Details
3.96 KB, patch
jgilbert
: review+
Details | Diff | Splinter Review
3.84 KB, patch
jgilbert
: review+
Details | Diff | Splinter Review
2.29 KB, patch
jgilbert
: review+
Details | Diff | Splinter Review
Attached file repro
<html>
  <head>
    <script>
      var gl = document.createElement('canvas').getContext('experimental-webgl')
      var texture = gl.createTexture()
      gl.bindTexture(gl.TEXTURE_2D, texture)
      gl.texImage2D(gl.TEXTURE_2D, 6, gl.RGBA, 512, 2, 0, gl.RGBA, gl.UNSIGNED_BYTE, null)
      gl.deleteTexture(texture)
    </script>
  </head>
</html>

crashes on linux with mesa like:

==32234== ERROR: AddressSanitizer crashed on unknown address 0x000100000011 (pc 0x7fffc5321690 sp 0x7fffffff4988 bp 0x000000000006 T0)
AddressSanitizer can not provide additional info. ABORTING
    #0 0x7fffc5321690 in ?? ??:0

also crashes chrome
Attached file asan log linux
OS: Mac OS X → Linux
I a debug build of Mesa, I get an assertion failure:

src/gallium/drivers/llvmpipe/lp_texture.c:395:llvmpipe_resource_map: Assertion `level == 0' failed.

Here, the value of level is 6 instead of the expected 0.


Program received signal SIGTRAP, Trace/breakpoint trap.
0x00007fffcc5bfb92 in _debug_assert_fail (expr=0x7fffccd9d3b0 "level == 0", file=0x7fffccd9d200 "src/gallium/drivers/llvmpipe/lp_texture.c", line=395, function=0x7fffccd9db30 "llvmpipe_resource_map")
    at src/gallium/auxiliary/util/u_debug.c:278
278           os_abort();
(gdb) bt
#0  0x00007fffcc5bfb92 in _debug_assert_fail (expr=0x7fffccd9d3b0 "level == 0", file=0x7fffccd9d200 "src/gallium/drivers/llvmpipe/lp_texture.c", line=395, function=0x7fffccd9db30 "llvmpipe_resource_map")
    at src/gallium/auxiliary/util/u_debug.c:278
#1  0x00007fffcc26a985 in llvmpipe_resource_map (resource=0x7fffd3bca000, level=6, layer=0, tex_usage=LP_TEX_USAGE_READ_WRITE, layout=LP_TEX_LAYOUT_LINEAR) at src/gallium/drivers/llvmpipe/lp_texture.c:395
#2  0x00007fffcc26b39d in llvmpipe_transfer_map (pipe=0x7fffde04c000, transfer=0x7fffde06bc00) at src/gallium/drivers/llvmpipe/lp_texture.c:699
#3  0x00007fffcc417088 in pipe_transfer_map (context=0x7fffde04c000, transfer=0x7fffde06bc00) at src/gallium/auxiliary/util/u_inlines.h:428
#4  0x00007fffcc417715 in st_texture_image_map (st=0x7fffd4e67000, stImage=0x7fffd44f3320, zoffset=0, usage=258, x=0, y=0, w=512, h=2) at src/mesa/state_tracker/st_texture.c:238
#5  0x00007fffcc40d76e in st_MapTextureImage (ctx=0x7fffd3a6d000, texImage=0x7fffd44f3320, slice=0, x=0, y=0, w=512, h=2, mode=6, mapOut=0x7fffffff8c08, rowStrideOut=0x7fffffff8c1c) at src/mesa/state_tracker/st_cb_texture.c:187
#6  0x00007fffcc3cafe3 in store_texsubimage (ctx=0x7fffd3a6d000, texImage=0x7fffd44f3320, xoffset=0, yoffset=0, zoffset=0, width=512, height=2, depth=1, format=6408, type=5121, pixels=0x7fffdda7d000, packing=0x7fffd3a73888, 
    caller=0x7fffccdec94a "glTexImage") at src/mesa/main/texstore.c:4352
#7  0x00007fffcc3cb206 in _mesa_store_teximage (ctx=0x7fffd3a6d000, dims=2, texImage=0x7fffd44f3320, format=6408, type=5121, pixels=0x7fffdda7d000, packing=0x7fffd3a73888) at src/mesa/main/texstore.c:4408
#8  0x00007fffcc40e13d in st_TexImage (ctx=0x7fffd3a6d000, dims=2, texImage=0x7fffd44f3320, format=6408, type=5121, pixels=0x7fffdda7d000, unpack=0x7fffd3a73888) at src/mesa/state_tracker/st_cb_texture.c:537
#9  0x00007fffcc3b6183 in teximage (ctx=0x7fffd3a6d000, dims=2, target=3553, level=6, internalFormat=6408, width=512, height=2, depth=1, border=0, format=6408, type=5121, pixels=0x7fffdda7d000) at src/mesa/main/teximage.c:2623
#10 0x00007fffcc3b6363 in _mesa_TexImage2D (target=3553, level=6, internalFormat=6408, width=512, height=2, border=0, format=6408, type=5121, pixels=0x7fffdda7d000) at src/mesa/main/teximage.c:2663
#11 0x00007ffff365a06f in mozilla::gl::GLContext::fTexImage2D (this=0x7fffd3a50000, target=3553, level=6, internalformat=6408, width=512, height=2, border=0, format=6408, type=5121, pixels=0x7fffdda7d000)
    at ../../../dist/include/GLContext.h:2672
#12 0x00007ffff3672ced in mozilla::WebGLContext::CheckedTexImage2D (this=0x7fffce86d400, target=3553, level=6, internalFormat=6408, width=512, height=2, border=0, format=6408, type=5121, data=0x7fffdda7d000)
    at /hack/mozilla-inbound/content/canvas/src/WebGLContextGL.cpp:5604
#13 0x00007ffff3673465 in mozilla::WebGLContext::TexImage2D_base (this=0x7fffce86d400, target=3553, level=6, internalformat=6408, width=512, height=2, srcStrideOrZero=0, border=0, format=6408, type=5121, data=0x0, byteLength=0, 
    jsArrayType=-1, srcFormat=mozilla::WebGLTexelConversions::Auto, srcPremultiplied=false) at /hack/mozilla-inbound/content/canvas/src/WebGLContextGL.cpp:5745
#14 0x00007ffff36736d1 in mozilla::WebGLContext::TexImage2D (this=0x7fffce86d400, cx=0x7fffd4394400, target=3553, level=6, internalformat=6408, width=512, height=2, border=0, format=6408, type=5121, pixels=0x0, rv=...)
Replacing 6 by 0 in the testcases avoids the crash, so this is specifically about setting a mipmap level >0 on a newly created texture.
Summary: firefox crashes with webgl, mesa → Mesa crashes when setting mipmap level>0 on a newly created texture
Crash confirmed on Mesa/Intel driver on another machine, so it's not just LLVMpipe.
Obviously the fact that the Mesa debug build asserts means it can't be used to assess how security critical this is.

In a release build of mesa, in valgrind, I get an invalid write, followed by an invalid read, and then the crash:

==26671== Invalid write of size 8
==26671==    at 0x2C92A723: llvmpipe_resource_create (lp_texture.c:165)
==26671==    by 0x2CA8E71B: st_texture_create (st_texture.c:94)
==26671==    by 0x2CA879B0: guess_and_alloc_texture (st_cb_texture.c:393)
==26671==    by 0x2CA89220: st_AllocTextureImageBuffer (st_cb_texture.c:447)
==26671==    by 0x2CA52461: _mesa_store_teximage (texstore.c:4403)
==26671==    by 0x2CA3C07E: teximage (teximage.c:2623)
==26671==    by 0x2CA3D2E7: _mesa_TexImage2D (teximage.c:2663)
==26671==    by 0x72E306E: mozilla::gl::GLContext::fTexImage2D(unsigned int, int, int, int, int, int, unsigned int, unsigned int, void const*) (GLContext.h:2672)
==26671==    by 0x72FBCEC: mozilla::WebGLContext::CheckedTexImage2D(unsigned int, int, unsigned int, int, int, int, unsigned int, unsigned int, void const*) (WebGLContextGL.cpp:5604)
==26671==    by 0x72FC464: mozilla::WebGLContext::TexImage2D_base(unsigned int, int, unsigned int, int, int, int, int, unsigned int, unsigned int, void*, unsigned int, int, mozilla::WebGLTexelConversions::WebGLTexelFormat, bool) (WebGLContextGL.cpp:5745)
==26671==    by 0x72FC6D0: mozilla::WebGLContext::TexImage2D(JSContext*, unsigned int, int, unsigned int, int, int, int, unsigned int, unsigned int, mozilla::dom::TypedArray_base<unsigned char, &(JS_GetObjectAsArrayBufferView(JSContext*, JSObject*, unsigned int*, unsigned char**))>*, mozilla::ErrorResult&) (WebGLContextGL.cpp:5789)
==26671==    by 0x83B6645: mozilla::dom::WebGLRenderingContextBinding::texImage2D(JSContext*, JS::Handle<JSObject*>, mozilla::WebGLContext*, unsigned int, JS::Value*) (WebGLRenderingContextBinding.cpp:4307)
==26671==  Address 0x1a93b7d8 is 0 bytes after a block of size 664 alloc'd
==26671==    at 0x402B7FA: calloc (vg_replace_malloc.c:591)
==26671==    by 0x2C92A352: llvmpipe_resource_create (lp_texture.c:244)
==26671==    by 0x2CA8E71B: st_texture_create (st_texture.c:94)
==26671==    by 0x2CA879B0: guess_and_alloc_texture (st_cb_texture.c:393)
==26671==    by 0x2CA89220: st_AllocTextureImageBuffer (st_cb_texture.c:447)
==26671==    by 0x2CA52461: _mesa_store_teximage (texstore.c:4403)
==26671==    by 0x2CA3C07E: teximage (teximage.c:2623)
==26671==    by 0x2CA3D2E7: _mesa_TexImage2D (teximage.c:2663)
==26671==    by 0x72E306E: mozilla::gl::GLContext::fTexImage2D(unsigned int, int, int, int, int, int, unsigned int, unsigned int, void const*) (GLContext.h:2672)
==26671==    by 0x72FBCEC: mozilla::WebGLContext::CheckedTexImage2D(unsigned int, int, unsigned int, int, int, int, unsigned int, unsigned int, void const*) (WebGLContextGL.cpp:5604)
==26671==    by 0x72FC464: mozilla::WebGLContext::TexImage2D_base(unsigned int, int, unsigned int, int, int, int, int, unsigned int, unsigned int, void*, unsigned int, int, mozilla::WebGLTexelConversions::WebGLTexelFormat, bool) (WebGLContextGL.cpp:5745)
==26671==    by 0x72FC6D0: mozilla::WebGLContext::TexImage2D(JSContext*, unsigned int, int, unsigned int, int, int, int, unsigned int, unsigned int, mozilla::dom::TypedArray_base<unsigned char, &(JS_GetObjectAsArrayBufferView(JSContext*, JSObject*, unsigned int*, unsigned char**))>*, mozilla::ErrorResult&) (WebGLContextGL.cpp:5789)
==26671== 
==26671== Invalid read of size 8
==26671==    at 0x2C9940D0: xlib_displaytarget_map (xlib_sw_winsys.c:231)
==26671==    by 0x2C92B0F4: llvmpipe_transfer_map (lp_texture.c:399)
==26671==    by 0x2CA8E961: st_texture_image_map (u_inlines.h:428)
==26671==    by 0x2CA875CC: st_MapTextureImage (st_cb_texture.c:187)
==26671==    by 0x2CA472DE: store_texsubimage (texstore.c:4352)
==26671==    by 0x2CA524B7: _mesa_store_teximage (texstore.c:4408)
==26671==    by 0x2CA3C07E: teximage (teximage.c:2623)
==26671==    by 0x2CA3D2E7: _mesa_TexImage2D (teximage.c:2663)
==26671==    by 0x72E306E: mozilla::gl::GLContext::fTexImage2D(unsigned int, int, int, int, int, int, unsigned int, unsigned int, void const*) (GLContext.h:2672)
==26671==    by 0x72FBCEC: mozilla::WebGLContext::CheckedTexImage2D(unsigned int, int, unsigned int, int, int, int, unsigned int, unsigned int, void const*) (WebGLContextGL.cpp:5604)
==26671==    by 0x72FC464: mozilla::WebGLContext::TexImage2D_base(unsigned int, int, unsigned int, int, int, int, int, unsigned int, unsigned int, void*, unsigned int, int, mozilla::WebGLTexelConversions::WebGLTexelFormat, bool) (WebGLContextGL.cpp:5745)
==26671==    by 0x72FC6D0: mozilla::WebGLContext::TexImage2D(JSContext*, unsigned int, int, unsigned int, int, int, int, unsigned int, unsigned int, mozilla::dom::TypedArray_base<unsigned char, &(JS_GetObjectAsArrayBufferView(JSContext*, JSObject*, unsigned int*, unsigned char**))>*, mozilla::ErrorResult&) (WebGLContextGL.cpp:5789)
==26671==  Address 0x100000011 is not stack'd, malloc'd or (recently) free'd
==26671==
If I comment out the assert that we hit in comment 4, I get a crash in the debug build at the same location as the invalid read crash in comment 7, with some information:

Program received signal SIGSEGV, Segmentation fault.
0x00007fffcf216b14 in xlib_displaytarget_map (ws=0x7fffd3eb0ab0, dt=0x100000001, flags=3) at src/gallium/winsys/sw/xlib/xlib_sw_winsys.c:231
231        xlib_dt->mapped = xlib_dt->data;

(gdb) bt
#0  0x00007fffcf216b14 in xlib_displaytarget_map (ws=0x7fffd3eb0ab0, dt=0x100000001, flags=3) at src/gallium/winsys/sw/xlib/xlib_sw_winsys.c:231
#1  0x00007fffcf1899a8 in llvmpipe_resource_map (resource=0x7fffddf37800, level=6, layer=0, tex_usage=LP_TEX_USAGE_READ_WRITE, layout=LP_TEX_LAYOUT_LINEAR) at src/gallium/drivers/llvmpipe/lp_texture.c:399
#2  0x00007fffcf18a378 in llvmpipe_transfer_map (pipe=0x7fffd4913000, transfer=0x7fffd48e1740) at src/gallium/drivers/llvmpipe/lp_texture.c:699
#3  0x00007fffcf336068 in pipe_transfer_map (context=0x7fffd4913000, transfer=0x7fffd48e1740) at src/gallium/auxiliary/util/u_inlines.h:428
#4  0x00007fffcf3366f5 in st_texture_image_map (st=0x7fffd42f6000, stImage=0x7fffd3ebe6a0, zoffset=0, usage=258, x=0, y=0, w=512, h=2) at src/mesa/state_tracker/st_texture.c:238
#5  0x00007fffcf32c74e in st_MapTextureImage (ctx=0x7fffd2ad6000, texImage=0x7fffd3ebe6a0, slice=0, x=0, y=0, w=512, h=2, mode=6, mapOut=0x7fffffff8c08, rowStrideOut=0x7fffffff8c1c) at src/mesa/state_tracker/st_cb_texture.c:187
#6  0x00007fffcf2e9fc3 in store_texsubimage (ctx=0x7fffd2ad6000, texImage=0x7fffd3ebe6a0, xoffset=0, yoffset=0, zoffset=0, width=512, height=2, depth=1, format=6408, type=5121, pixels=0x7fffd51fa000, packing=0x7fffd2adc888, 
    caller=0x7fffcfd0b92a "glTexImage") at src/mesa/main/texstore.c:4352
#7  0x00007fffcf2ea1e6 in _mesa_store_teximage (ctx=0x7fffd2ad6000, dims=2, texImage=0x7fffd3ebe6a0, format=6408, type=5121, pixels=0x7fffd51fa000, packing=0x7fffd2adc888) at src/mesa/main/texstore.c:4408
#8  0x00007fffcf32d11d in st_TexImage (ctx=0x7fffd2ad6000, dims=2, texImage=0x7fffd3ebe6a0, format=6408, type=5121, pixels=0x7fffd51fa000, unpack=0x7fffd2adc888) at src/mesa/state_tracker/st_cb_texture.c:537
#9  0x00007fffcf2d5163 in teximage (ctx=0x7fffd2ad6000, dims=2, target=3553, level=6, internalFormat=6408, width=512, height=2, depth=1, border=0, format=6408, type=5121, pixels=0x7fffd51fa000) at src/mesa/main/teximage.c:2623
#10 0x00007fffcf2d5343 in _mesa_TexImage2D (target=3553, level=6, internalFormat=6408, width=512, height=2, border=0, format=6408, type=5121, pixels=0x7fffd51fa000) at src/mesa/main/teximage.c:2663
#11 0x00007ffff365a06f in mozilla::gl::GLContext::fTexImage2D (this=0x7fffde5f9000, target=3553, level=6, internalformat=6408, width=512, height=2, border=0, format=6408, type=5121, pixels=0x7fffd51fa000)
    at ../../../dist/include/GLContext.h:2672

(gdb) p xlib_dt
$1 = (struct xlib_displaytarget *) 0x100000001

If this 0x100000001 is really fixed and can't be manipulated, then we're safe. Is it?
on intel with attachment 655907 [details] I can get atleast:

==14260== ERROR: AddressSanitizer crashed on unknown address 0x00005c008000 (pc 0x7fffb70fc1c4 sp 0x7fffffff48e0 bp 0x000000000007 T0)
==15327== ERROR: AddressSanitizer crashed on unknown address 0x000000000008 (pc 0x7fffb78d296b sp 0x7fffffff4810 bp 0x7fffcba1d880 T0)
==10880== ERROR: AddressSanitizer crashed on unknown address 0x000016004000 (pc 0x7fffb78fc48f sp 0x7fffffff48e0 bp 0x000000000000 T0)
==7359== ERROR: AddressSanitizer crashed on unknown address 0x000017002010 (pc 0x7ffff6f37e64 sp 0x7fffffff45e8 bp 0x7fffffff4840 T0)
==8053== ERROR: AddressSanitizer crashed on unknown address 0x00002e004010 (pc 0x7ffff6f37e64 sp 0x7fffffff45d8 bp 0x7fffffff4830 T0)
==11526== ERROR: AddressSanitizer crashed on unknown address 0x000000000000 (pc 0x7fffb74a9c39 sp 0x7fffffff4848 bp 0x7ffff6b4c080 T0)
==12790== ERROR: AddressSanitizer crashed on unknown address 0x00005e004010 (pc 0x7ffff6f37e64 sp 0x7fffffff45e8 bp 0x7fffffff4840 T0)
==13532== ERROR: AddressSanitizer crashed on unknown address 0x000078010000 (pc 0x7fffb78fc17a sp 0x7fffffff48e0 bp 0x000000000003 T0)
Hah. So this bug looks security-sensitive depending on the driver: sec-sensitive on the Intel driver at least (comment 12) but apparently not on LLVMpipe driver (comment 8).
This fixes it at least for the present testcase.
Attachment #656157 - Flags: review?
Attachment #656157 - Flags: review? → review?(jgilbert)
It's worth checking if a similar bug exists in glCopyTexImage2D.
Comment on attachment 656157 [details] [diff] [review]
work around the mesa teximage2d crasher by rejecting certain legal, but rare, teximage2d calls

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

Gross, but necessary. Does this show up as a failure when the conformance suite is run? If not, we should add a test to make sure it does. We don't want to forget about something subtle (and hopefully temporary!) like this.
Attachment #656157 - Flags: review?(jgilbert) → review+
We should actually be able to work around this in most cases, but it's probably not worth the effort when it should instead be fixed in Mesa.
Marking sec-critical during sec triage based on comment 12 addresses.
Keywords: sec-critical
You are right jgilbert, adding webgl-test-needed. I haven't actually checked if this is caught by the trunk test suite but let's default to assuming it isn't.

I am not convinced though that it really matters. If it does, rather than this fix, we should instead insert dummy texImage2D calls to allocate the level 0 images.
Whiteboard: webgl-test-needed
(In reply to Benoit Jacob [:bjacob] from comment #19)
> You are right jgilbert, adding webgl-test-needed. I haven't actually checked
> if this is caught by the trunk test suite but let's default to assuming it
> isn't.
> 
> I am not convinced though that it really matters. If it does, rather than
> this fix, we should instead insert dummy texImage2D calls to allocate the
> level 0 images.

We are lucky that the workaround is simply uploading a zero'd texture. Since GLES requires level-0 to be specified for the texture to be complete, we can just emulate it being incomplete with zero'd level-0 data.
Whiteboard: webgl-test-needed → [asan] webgl-test-needed
Assignee: nobody → bjacob
(In reply to Benoit Jacob [:bjacob] from comment #15)
> It's worth checking if a similar bug exists in glCopyTexImage2D.

Checked: there is a similar bug with glCopyTexImage2D. Attaching corresponding testcase.
Since it's a mesa crash we're working around and these are pretty basic functions I assume it affects all old branches. If so please remove the regressionwindow-wanted flag and set the appropriate branch status flags to "affected".
... and I have checked compressedTexImage2D too (with a DXT1 texture): it does NOT trigger the crash.

Going to do the above-discussed workaround which should be cheap enough (we should be able to get away with a 1x1 dummy texture) so we'll be able to just keep the workaround for all Mesa versions, rather than having to track Mesa versions.
Summary: Mesa crashes when setting mipmap level>0 on a newly created texture → Mesa crashes on certain texImage2D calls involving level>0
This new testcase nullifies the above patch -- it crashes even with this patch, in fact, it falls outside of the scope of what that patch works around.

APItrace:

453 glGenTextures(n = 1, textures = &2)
455 glBindTexture(target = GL_TEXTURE_2D, texture = 2)
458 glTexImage2D(target = GL_TEXTURE_2D, level = 0, internalformat = GL_RGBA, width = 512, height = 2, border = 0, format = GL_RGBA, type = GL_UNSIGNED_BYTE, pixels = blob(4096))
462 glTexImage2D(target = GL_TEXTURE_2D, level = 1, internalformat = GL_RGBA, width = 512, height = 2, border = 0, format = GL_RGBA, type = GL_UNSIGNED_BYTE, pixels = blob(4096))
466 glTexImage2D(target = GL_TEXTURE_2D, level = 2, internalformat = GL_RGBA, width = 512, height = 2, border = 0, format = GL_RGBA, type = GL_UNSIGNED_BYTE, pixels = blob(4096))
470 glTexImage2D(target = GL_TEXTURE_2D, level = 3, internalformat = GL_RGBA, width = 512, height = 2, border = 0, format = GL_RGBA, type = GL_UNSIGNED_BYTE, pixels = blob(4096))
474 glTexImage2D(target = GL_TEXTURE_2D, level = 4, internalformat = GL_RGBA, width = 512, height = 2, border = 0, format = GL_RGBA, type = GL_UNSIGNED_BYTE, pixels = blob(4096))
478 glTexImage2D(target = GL_TEXTURE_2D, level = 5, internalformat = GL_RGBA, width = 512, height = 2, border = 0, format = GL_RGBA, type = GL_UNSIGNED_BYTE, pixels = blob(4096)) // incomplete
Attachment #656157 - Attachment is obsolete: true
The crash condition appears to be:

  level>0  AND width>=512 AND height>=2 AND (mipmap level 0 undefined OR mipmap level 0 has the same width)
This prevents crashes in all the variants of the testcase that I've been able to make.
Attachment #661222 - Flags: review?(jgilbert)
Comment on attachment 661222 [details] [diff] [review]
work around the mesa teximage2d crasher by rejecting certain legal, but rare, teximage2d and copyTexImage2d calls

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

I could be misreading this, but it looks like the conditions we test are wrong.

::: content/canvas/src/WebGLContextGL.cpp
@@ +908,5 @@
> +        level > 0 &&
> +        !sub)
> +    {
> +        size_t face = WebGLTexture::FaceForTarget(target);
> +        WebGLTexture *tex = activeBoundTextureForTarget(target);

Is |tex| always going to be non-null?

@@ +910,5 @@
> +    {
> +        size_t face = WebGLTexture::FaceForTarget(target);
> +        WebGLTexture *tex = activeBoundTextureForTarget(target);
> +        if (!tex->HasImageInfoAt(0, face) ||
> +            tex->ImageInfoAt(0, face).Width() >= width)

This doesn't seem in line with the crash condition you listed: "level>0  AND width>=512 AND height>=2 AND (mipmap level 0 undefined OR mipmap level 0 has the same width)"

Also, |tex->ImageInfoAt(0, face).Width() >= width| seems like it would be very common, since level >0 should always have a smaller width than level 0.

@@ +913,5 @@
> +        if (!tex->HasImageInfoAt(0, face) ||
> +            tex->ImageInfoAt(0, face).Width() >= width)
> +        {
> +            return  ErrorInvalidOperation("%s: rejecting valid call to avoid Mesa driver crash. "
> +                                          "Please go complain on https://bugzilla.mozilla.org/show_bug.cgi?id=785734",

Why are we telling people to complain on this bug, especially since this bug is currently a sec-bug, and thus invisible to most? Should we not tell them to complain to Mesa?
Attachment #661222 - Flags: review?(jgilbert) → review-
Indeed: tex needed checking for non-null, and the inequality on width was backwards. I want to keep a reference to this bug so we get a chance to learn if the solution adopted here is harming real-world content (once this bug is un-hidden), but I simplified it.
Attachment #661222 - Attachment is obsolete: true
Attachment #663519 - Flags: review?(jgilbert)
Comment on attachment 663519 [details] [diff] [review]
work around the mesa teximage2d crasher by rejecting certain legal, but rare, teximage2d and copyTexImage2d calls

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

One formatting nit.

::: content/canvas/src/WebGLContextGL.cpp
@@ +678,5 @@
> +    {
> +        if (!tex->HasImageInfoAt(0, face) ||
> +            tex->ImageInfoAt(0, face).Width() <= width)
> +        {
> +            return  ErrorInvalidOperation("%s: rejecting valid call to avoid Mesa driver crash. "

You have an extra space between 'return' and 'ErrorInvalidOperation'.
Attachment #663519 - Flags: review?(jgilbert) → review+
https://hg.mozilla.org/mozilla-central/rev/a0bae754ad5d
Status: NEW → RESOLVED
Closed: 12 years ago
Resolution: --- → FIXED
Comment on attachment 663519 [details] [diff] [review]
work around the mesa teximage2d crasher by rejecting certain legal, but rare, teximage2d and copyTexImage2d calls

Sorry for the delayed request.

[Approval Request Comment]
Bug caused by (feature/regressing bug #): Driver bug
User impact if declined: sec-critical
Testing completed (on m-c, etc.): been on m-c since last week
Risk to taking this patch (and alternatives if risky): very low risk, worst case we'd break some WebGL page, but this has been in m-c for half a week without a report of regression
String or UUID changes made by this patch: none
Attachment #663519 - Flags: approval-mozilla-beta?
Attachment #663519 - Flags: approval-mozilla-aurora?
Comment on attachment 663519 [details] [diff] [review]
work around the mesa teximage2d crasher by rejecting certain legal, but rare, teximage2d and copyTexImage2d calls

This missed our final beta, and doesn't seem critical for release. Approving for Aurora 17, however.
Attachment #663519 - Flags: approval-mozilla-beta?
Attachment #663519 - Flags: approval-mozilla-beta-
Attachment #663519 - Flags: approval-mozilla-aurora?
Attachment #663519 - Flags: approval-mozilla-aurora+
Hey guys, are you sure a simple rejection of

  GLint maxSize = MaxSizeForTarget(target) >> level;

  if (width > maxSize || height > maxSize) {
     // generate GL error INVALID_VALUE

doesn't handle the issue as well? I tested this on Chrome and it seemed to stop the asan issues though I might have missed one.
Hm, so here I am testing with a debug build of Mesa and I'm testing whether the Mesa assertion fails rather than whether a memory corruption happens (because I am lazy).

The condition, here, is:

  GLint maxSize = 16384 >> level;

  if (width > maxSize || height > maxSize) {
     // generate GL error INVALID_VALUE

But MAX_TEXTURE_SIZE, as returned by Mesa, is 4096.

In any case, it does seem that the problem is symmetric between height and width, so I need to amend my patch in some way.
Reopening because the same issue occurs with height as it does with width -- not sure why I thought otherwise.
Status: RESOLVED → REOPENED
Resolution: FIXED → ---
More testing confirms comment 37 on my system; since your proposed restriction is stricter, it would indeed avoid the crash.
Attached patch correct patch (Gregg's idea) (obsolete) — Splinter Review
Attachment #668711 - Flags: review?(jgilbert)
I added a conformance test under the assumption that Daniel's interpretation of the ES spec is correct 

https://www.khronos.org/registry/webgl/sdk/tests/conformance/textures/texture-size-limit.html
Thanks!

You're right, indeed, in the GLES 2.0.25 spec page 69 it says that we should return INVALID_VALUE in this case.

Then I'll update the patch to 1) generate INVALID_VALUE and 2) do this check unconditionally, not just as a driver work-around.
Attachment #668711 - Flags: review?(jgilbert)
Attachment #668711 - Attachment is obsolete: true
Attachment #668732 - Flags: review?(jgilbert)
Comment on attachment 668732 [details] [diff] [review]
correct patch (Gregg's idea)

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

Interesting.
Attachment #668732 - Flags: review?(jgilbert) → review+
Comment on attachment 668732 [details] [diff] [review]
correct patch (Gregg's idea)

[Approval Request Comment]
Bug caused by (feature/regressing bug #): driver bug
User impact if declined: sec-critical
Testing completed (on m-c, etc.): m-i
Risk to taking this patch (and alternatives if risky): no risk
String or UUID changes made by this patch: none
Attachment #668732 - Flags: approval-mozilla-beta?
Attachment #668732 - Flags: approval-mozilla-aurora?
https://hg.mozilla.org/mozilla-central/rev/81675e760c12

(I reset the status flags based on what I can tell happened here. Please change them back if I goofed)
Status: REOPENED → RESOLVED
Closed: 12 years ago12 years ago
Flags: in-testsuite?
Resolution: --- → FIXED
Target Milestone: mozilla18 → mozilla19
Attachment #668732 - Flags: approval-mozilla-beta?
Attachment #668732 - Flags: approval-mozilla-beta+
Attachment #668732 - Flags: approval-mozilla-aurora?
Attachment #668732 - Flags: approval-mozilla-aurora+
(In reply to Benoit Jacob [:bjacob] from comment #48)
> http://hg.mozilla.org/releases/mozilla-aurora/rev/fe15e7126620
> http://hg.mozilla.org/releases/mozilla-beta/rev/6feb23ee423c

Please prepare a patch for mozilla-esr10 as well. Thanks!
Attached patch esr10 patchSplinter Review
I threw into this patch the fix for the same issue in copyTexImage2D, bug 802778.
Attachment #675620 - Flags: review?(jgilbert)
Attachment #675620 - Flags: review?(jgilbert) → review+
Comment on attachment 675620 [details] [diff] [review]
esr10 patch

[Approval Request Comment]
If this is not a sec:{high,crit} bug, please state case for ESR consideration:
User impact if declined: 
Fix Landed on Version:
Risk to taking this patch (and alternatives if risky): 
String or UUID changes made by this patch: 

See https://wiki.mozilla.org/Release_Management/ESR_Landing_Process for more info.
Attachment #675620 - Flags: approval-mozilla-esr10?
Erm, again:

[Approval Request Comment]
If this is not a sec:{high,crit} bug, please state case for ESR consideration: sec:critical
User impact if declined: sec:critical
Fix Landed on Version: 17+
Risk to taking this patch (and alternatives if risky): very low risk (simple, well tested already)
String or UUID changes made by this patch: none
Whiteboard: [asan] webgl-test-needed → [asan][adv-track-main17+] webgl-test-needed
Attachment #675620 - Flags: approval-mozilla-esr10? → approval-mozilla-esr10+
Whiteboard: [asan][adv-track-main17+] webgl-test-needed → [asan][adv-track-main17+][adv-track-esr17+] webgl-test-needed
Alias: CVE-2012-5833
Keywords: verifyme
Can miaubiz@gmail.com take a look and verify fixed on these config(s)? I currently don't have access to these particular configs in order to confirm. Thank you.
Swapping out verifyme for [qa-] given comment 54.
Keywords: verifyme
Whiteboard: [asan][adv-track-main17+][adv-track-esr17+] webgl-test-needed → [asan][adv-track-main17+][adv-track-esr17+][qa-] webgl-test-needed
Matt Wobensmith: how do I verify? it's fixed for me atleast :|
(In reply to miaubiz from comment #56)
> Matt Wobensmith: how do I verify? it's fixed for me atleast :|

If you cannot reproduce this crash anymore then I think it's safe to call this verified. Can you please confirm the following builds don't crash:
* Firefox 17.0.2
* Firefox 18.0.1
* Firefox 19.0b2
* Firefox 17.0.2esr
* Firefox 10.0.12esr
(listed in order of priority)

Thank you
couldn't find a firefox 17.0.2 non-esr

18.0 and 19.0b2 both say "Error: WebGL: texImage2D: the maximum texture size for level 6 is 128", as expected.

cheers
17.0.1 and 17.0.2esr give the same correct error message

10.0.12esr can't get a webgl context, presumably because webgl was blacklisted in 10.0.12esr,  it is therefore not affected by the bug.
miaubiz, your time spent verifying this is very much appreciated!

Mesa is blacklisted in ESR10 as you noticed in comment 60.
Thank you very much, miaubuz. Marking this verified across all branches.
Group: core-security
rforbes-bugspam-for-setting-that-bounty-flag-20130719
Flags: sec-bounty+
You need to log in before you can comment on or make changes to this bug.