Last Comment Bug 744888 - (CVE-2012-3105) Work around NVIDIA driver bug in glBufferData
(CVE-2012-3105)
: Work around NVIDIA driver bug in glBufferData
Status: RESOLVED FIXED
[qa-][advisory-tracking+]
: sec-vector
Product: Core
Classification: Components
Component: Canvas: WebGL (show other bugs)
: unspecified
: All All
: -- normal (vote)
: mozilla14
Assigned To: Benoit Jacob [:bjacob] (mostly away)
:
Mentors:
Depends on:
Blocks:
  Show dependency treegraph
 
Reported: 2012-04-12 12:12 PDT by Benoit Jacob [:bjacob] (mostly away)
Modified: 2012-08-02 13:04 PDT (History)
8 users (show)
See Also:
Crash Signature:
(edit)
QA Whiteboard:
Iteration: ---
Points: ---
Has Regression Range: ---
Has STR: ---
wontfix
fixed
fixed
13+
fixed


Attachments
work around nvidia bufferdata bug (1.38 KB, patch)
2012-04-12 14:31 PDT, Benoit Jacob [:bjacob] (mostly away)
jgilbert: review-
Details | Diff | Splinter Review
like this instead? (1.32 KB, patch)
2012-04-13 11:43 PDT, Benoit Jacob [:bjacob] (mostly away)
jgilbert: review+
akeybl: approval‑mozilla‑aurora+
akeybl: approval‑mozilla‑beta-
lukasblakk+bugs: approval‑mozilla‑esr10+
Details | Diff | Splinter Review
CanvasMatrix.js to use with the testcase (10.85 KB, application/javascript)
2012-05-14 15:00 PDT, Benoit Jacob [:bjacob] (mostly away)
no flags Details
testcase: unknown-2.html (4.18 KB, text/html)
2012-05-14 15:02 PDT, Benoit Jacob [:bjacob] (mostly away)
no flags Details

Description Benoit Jacob [:bjacob] (mostly away) 2012-04-12 12:12:35 PDT
Thanks to Ken/Google for letting us know about this. My understanding of

  http://code.google.com/p/chromium/issues/detail?id=118970

is that the NVIDIA drivers have a bug: glBufferData with null data doesn't actually allocate the buffer until actual data is uploaded to it.

This affects WebGL. We could work around this in 2 different places in WebGL (in WebGL bufferData and in the attrib 0 emulation code), but instead, I think that this is best handled at the level of GLContext.
Comment 1 Benoit Jacob [:bjacob] (mostly away) 2012-04-12 14:31:02 PDT
Created attachment 614571 [details] [diff] [review]
work around nvidia bufferdata bug
Comment 2 Jeff Gilbert [:jgilbert] 2012-04-13 07:32:23 PDT
Patch looks fine, and should fix the problem, but it probably fixes it too conservatively. This forces us to allocate memory, zero, and upload a buffer of the full size when what is being asked for is merely buffer allocation. If we're just trying to assure the buffer is allocated and sized properly, we should just be able to upload one byte at the very end of the buffer.
Comment 3 Jeff Gilbert [:jgilbert] 2012-04-13 07:33:34 PDT
Comment on attachment 614571 [details] [diff] [review]
work around nvidia bufferdata bug

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

Moving to r- pending discussion and/or clarification.
Comment 4 Kenneth Russell 2012-04-13 08:02:46 PDT
Note that when Chrome's GPU subsystem receives a glBufferData call from an untrusted client with NULL as the input, it explicitly allocates a large zeroed buffer and uploads it to the GPU. See GLES2DecoderImpl::DoBufferData, http://src.chromium.org/viewvc/chrome/trunk/src/gpu/command_buffer/service/gles2_cmd_decoder.cc?view=markup .
Comment 5 Benoit Jacob [:bjacob] (mostly away) 2012-04-13 08:23:18 PDT
Ken: thanks, the notion a 'trusted client' at the level of the GL wrapper is quite interesting. Maybe we should do the same.
Comment 6 Jeff Gilbert [:jgilbert] 2012-04-13 08:31:40 PDT
webgl.bufferData(null) is disallowed, but I don't think we care otherwise. I think the important issue here is just to be sure buffers are allocated when we ask them to be. Ideally, we would just defer this until we (maybe) need them, and (maybe) assure allocation then. (ideally warning if we are using uninitialized buffers, though it should only help us internally, and we shouldn't be having any problems there)

I need to read through the chromium bug again when I'm more coherent.

After re-reading it anyways, I think I understand better. Part of the root of the problem is that the driver 'consumes'(reads) all enabled vertex arrays, even if the in-use program does not read from them. (Which I was certainly not aware of) If a too-short (or in this driver's case, lazily-thus-not-yet-allocated) buffer is bound to an enabled vertex pointer, the driver can be accessing uninitialized/invalid memory.

I think I'll need to read through our implementation and trace this out for myself.
Comment 7 Benoit Jacob [:bjacob] (mostly away) 2012-04-13 11:43:24 PDT
Created attachment 614867 [details] [diff] [review]
like this instead?
Comment 8 Jeff Gilbert [:jgilbert] 2012-04-14 02:49:15 PDT
Comment on attachment 614867 [details] [diff] [review]
like this instead?

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

This is what seems like a more minimal change, to me.

::: gfx/gl/GLContext.h
@@ +2014,5 @@
> +        if (WorkAroundDriverBugs() &&
> +            !data &&
> +            Vendor() == VendorNVIDIA)
> +        {
> +            char c;

You might want to initialize this to keep everything happy about uninitialized variables being used. (Not that it should matter in practice, but still)
Comment 9 Benoit Jacob [:bjacob] (mostly away) 2012-04-16 12:58:36 PDT
http://hg.mozilla.org/mozilla-central/rev/e74b044c18b8
Comment 10 Benoit Jacob [:bjacob] (mostly away) 2012-04-18 10:37:18 PDT
Comment on attachment 614867 [details] [diff] [review]
like this instead?

[Approval Request Comment]
Regression caused by (bug #): We've had this problem since WebGL shipped in Firefox 4
User impact if declined: security issue: information leakage vulnerability with NVIDIA driver
Testing completed (on m-c, etc.): been on m-c for a few days.
Risk to taking this patch (and alternatives if risky): Does not seem risky. Could be a tiny performance overhead. Could fail to fix the problem. If we run into another unforeseen driver bug, it could even lead to a crash, but we don't have any definite reason to think that such a driver bug is waiting for us.
String changes made by this patch: none
Comment 11 Alex Keybl [:akeybl] 2012-04-19 14:29:50 PDT
Comment on attachment 614867 [details] [diff] [review]
like this instead?

[Triage Comment]
We weighed the risk and reward of this nomination and decided that this <sg:high issue was not worth landing between beta 6 and our final build. Approving for Aurora 13, however.
Comment 12 Benoit Jacob [:bjacob] (mostly away) 2012-04-23 12:29:11 PDT
http://hg.mozilla.org/releases/mozilla-aurora/rev/9edd931bf07f
Comment 13 Benoit Jacob [:bjacob] (mostly away) 2012-04-23 13:05:39 PDT
bustage fix:
http://hg.mozilla.org/releases/mozilla-aurora/rev/f53d4a882978
Comment 14 Anthony Hughes (:ashughes) [GFX][QA][Mentor] 2012-04-26 14:28:54 PDT
Is there a testcase QA can use to verify this fix?
Comment 15 Anthony Hughes (:ashughes) [GFX][QA][Mentor] 2012-05-10 12:12:08 PDT
(In reply to Anthony Hughes, Mozilla QA (irc: ashughes) from comment #14)
> Is there a testcase QA can use to verify this fix?

Bump.
Comment 16 Benoit Jacob [:bjacob] (mostly away) 2012-05-10 12:16:34 PDT
The original Chromium bug has a testcase:
http://code.google.com/p/chromium/issues/detail?id=118970
Comment 17 Anthony Hughes (:ashughes) [GFX][QA][Mentor] 2012-05-10 15:27:23 PDT
Thanks Benoit.
Comment 18 Lukas Blakk [:lsblakk] use ?needinfo 2012-05-14 13:38:19 PDT
Assigning to Benoit to get this nominated, and landed to ESR.
Comment 19 Benoit Jacob [:bjacob] (mostly away) 2012-05-14 14:25:49 PDT
Comment on attachment 614867 [details] [diff] [review]
like this instead?

[Approval Request Comment]
If this is not a sec:{high,crit} bug, please state case for ESR consideration: Well, this bug allows exploiting an information leakage vulnerability in NVIDIA drivers, that would probably rate sg:high
User impact if declined: information leakage vulnerability with NVIDIA driver
Fix Landed on Version: 14
Risk to taking this patch (and alternatives if risky): low risk (see above aurora approval request)
String changes made by this patch: none
Comment 20 Al Billings [:abillings] 2012-05-14 14:53:20 PDT
Anthony, do you have a copy of the testcase? I have no access to http://code.google.com/p/chromium/issues/detail?id=118970.
Comment 21 Benoit Jacob [:bjacob] (mostly away) 2012-05-14 15:00:54 PDT
Created attachment 623837 [details]
CanvasMatrix.js to use with the testcase
Comment 22 Benoit Jacob [:bjacob] (mostly away) 2012-05-14 15:02:33 PDT
Created attachment 623838 [details]
testcase: unknown-2.html

Here's the testcase. The chromium bug says:

REPRODUCTION CASE
1. Download both files (nothing wrong with CanvasMatrix.js)
2. chrome-asan unknown-2.html
3. gpu process crashes
Comment 23 Lukas Blakk [:lsblakk] use ?needinfo 2012-05-16 11:36:32 PDT
Comment on attachment 614867 [details] [diff] [review]
like this instead?

low risk, sg:high and fix already on mozilla-beta (13), approving for ESR landing.
Comment 24 Al Billings [:abillings] 2012-05-18 15:59:36 PDT
Running this testcase against a Firefox 12 on my Win 7 x64 box, which has an nvdia crash, I don't have any issues. Does this only reproduce on certain operating systems or in ASAN builds (of which we have none on Windows)?
Comment 25 Al Billings [:abillings] 2012-05-18 17:29:22 PDT
(In reply to Lukas Blakk [:lsblakk] from comment #23)
> low risk, sg:high and fix already on mozilla-beta (13), approving for ESR
> landing.

If this is sg:high, why isn't it marked as such?
Comment 26 Benoit Jacob [:bjacob] (mostly away) 2012-05-20 14:13:32 PDT
(In reply to Al Billings [:abillings] from comment #25)
> (In reply to Lukas Blakk [:lsblakk] from comment #23)
> > low risk, sg:high and fix already on mozilla-beta (13), approving for ESR
> > landing.
> 
> If this is sg:high, why isn't it marked as such?

Maybe my views on what's sg:high aren't universally shared, or maybe it's just an oversight.

Patch ready for landing on ESR10 but tree is closed due to bug 756365.
Comment 27 Benoit Jacob [:bjacob] (mostly away) 2012-05-21 15:07:50 PDT
https://hg.mozilla.org/releases/mozilla-esr10/rev/d740253129c7
Comment 28 Kevin Brosnan [:kbrosnan] 2012-06-02 10:29:34 PDT
I don't crash on Firefox 12 with the following NVIDIA driver/hardware so I am unable to verify this fix. Is there specific driver versions that are affected or hardware mentioned in the Chrome bug?

  Application Basics
        Name
        Firefox

        Version
        12.0

        User Agent
        Mozilla/5.0 (Windows NT 6.1; WOW64; rv:12.0) Gecko/20100101 Firefox/12.0

  Graphics

        Adapter Description
        NVIDIA GeForce GT 240M

        Vendor ID
        0x10de

        Device ID
        0x0a34

        Adapter RAM
        1024

        Adapter Drivers
        nvd3dumx,nvwgf2umx,nvwgf2umx nvd3dum,nvwgf2um,nvwgf2um

        Driver Version
        8.17.13.142

        Driver Date
        5-15-2012

        Direct2D Enabled
        true

        DirectWrite Enabled
        true (6.1.7601.17789)

        ClearType Parameters
        DISPLAY1 [ Gamma: 2200 Pixel Structure: RGB ClearType Level: 100 Enhanced Contrast: 400 ] DISPLAY2 [ Gamma: 2200 Pixel Structure: RGB ClearType Level: 50 Enhanced Contrast: 200 ]

        WebGL Renderer
        Google Inc. -- ANGLE (NVIDIA GeForce GT 240M) -- OpenGL ES 2.0 (ANGLE 1.0.0.963)

        GPU Accelerated Windows
        1/1 Direct3D 10

        AzureBackend
        direct2d
Comment 29 Anthony Hughes (:ashughes) [GFX][QA][Mentor] 2012-06-03 11:24:26 PDT
Benoit, this bug seems to be unreproducible on our end. Do you have any leads QA can pursue to test this fix? 

If there is someone else watching this bug who is able to test this, please help us out by verifying it is fixed. QA is seeking verification in Firefox 13, 10.0.5ESR and 14.0a2.

Thank you.
Comment 30 Benoit Jacob [:bjacob] (mostly away) 2012-06-03 14:37:57 PDT
It's alright, driver bugs are often tricky to reproduce. If you tried and couldn't reproduce, it's probably OK to stop there on this bug.
Comment 31 Anthony Hughes (:ashughes) [GFX][QA][Mentor] 2012-06-04 13:49:04 PDT
Marking [qa-] since QA is unable to reproduce the original issue. Please mark [qa+] is a more reproducible case is found so we can verify this is fixed.

Thanks

Note You need to log in before you can comment on or make changes to this bug.