Bug 59429 - brw_fs.cpp:1466: bool fs_visitor::remove_dead_constants(): Assertion `constant_nr < (int)c->prog_data.nr_params
Summary: brw_fs.cpp:1466: bool fs_visitor::remove_dead_constants(): Assertion `constan...
Status: RESOLVED FIXED
Alias: None
Product: Mesa
Classification: Unclassified
Component: Drivers/DRI/i965 (show other bugs)
Version: git
Hardware: x86-64 (AMD64) Linux (All)
: medium normal
Assignee: fjhenigman
QA Contact:
URL:
Whiteboard:
Keywords:
Depends on:
Blocks:
 
Reported: 2013-01-15 16:48 UTC by fjhenigman
Modified: 2013-06-06 22:05 UTC (History)
4 users (show)

See Also:
i915 platform:
i915 features:


Attachments
piglit shader_runner file containing problem shaders (226 bytes, text/plain)
2013-01-15 16:48 UTC, fjhenigman
Details
fix (1.62 KB, patch)
2013-01-16 22:13 UTC, fjhenigman
Details | Splinter Review
updated fix (1.64 KB, patch)
2013-05-22 05:45 UTC, Dave Airlie
Details | Splinter Review

Description fjhenigman 2013-01-15 16:48:42 UTC
Created attachment 73106 [details]
piglit shader_runner file containing problem shaders

assertion hit during shader linking
piglit shader_runner file attached
tested on sandy bridge / gen 6
Comment 1 fjhenigman 2013-01-16 22:13:20 UTC
Created attachment 73167 [details] [review]
fix
Comment 2 fjhenigman 2013-01-16 22:18:48 UTC
I think the problem is that after loop unrolling there's an out-of-bounds array access which fs_visitor::remove_dead_constants() does not expect so it hits an assert.
There is a check for out-of-bounds array access before this point, but I guess only before the shader code is transformed.
The patch changes the assertion failure to a compilation failure.
It also sets params_remap to NULL in the fs_visitor constructor since otherwise it seems the assert at brw_fs.cpp:1504 ("should have been generated in the 8-wide pass") could be testing an uninitialized value.
Comment 3 Huzaifa Sidhpurwala 2013-03-20 06:24:44 UTC
We have assigned CVE-2013-1872 to this security flaw.

I am aware that the easiest way to exploit this is via a browser using webgl.
Though mozilla and google have assigned CVEs, but that is because they impact the browser. The above CVE is assigned for its impact on MESA as shipped independently.

Is there a un-embargo date for this flaw?
Comment 4 Eric Anholt 2013-04-08 22:44:46 UTC
Given that a piglit test reproducing this bug was sent out, is there a reason for this bug report to be marked secret?  It's why I haven't looked at the fix for it.
Comment 5 Stephane Marchesin 2013-04-08 22:51:13 UTC
Only people in mesa security can alter the flag. So it's going to stay that way until someone from that group changes it.
Comment 6 Huzaifa Sidhpurwala 2013-04-09 05:30:33 UTC
It seems i can alter the flag as well (not that i want to).
The Bigger question here is can we consider this flaw to be public?, given that
Mozilla and Google have published advisories about this flaw affecting their product?

http://www.mozilla.org/security/announce/2013/mfsa2013-35.html
Comment 7 Benoit Jacob 2013-04-09 11:33:38 UTC
Given that a work-around for this bug was rolled out in Firefox 20 (and separately, in Firefox ESR), I have no issue with this bug being made public from a Mozilla perspective.

I _believe_ that in Chromium the work-around is deployed since version 25 so there should be no issue from the Chromium side either, but I'm not authoritative on this.

I do not know the complete list of WebGL-supporting browsers that may run into this driver bug.
Comment 8 Dave Airlie 2013-05-22 05:45:59 UTC
Created attachment 79636 [details] [review]
updated fix

this fix checks for < 0 values as well.
Comment 9 Dave Airlie 2013-05-22 05:46:49 UTC
the fix is incomplete

Okay this fix is incomplete, if you use negative constant numbers

  "uniform vec4 uni[1];\n"
  "void main()\n"
  "{\n"
  "  vec4 c = vec4(0,0,0,0);\n"
  "  for (int ii = -11; ii < -9; ++ii) {\n"
  "    c += uni[ii];\n"
  "  }\n"
  "  gl_FragColor = vec4(c.r, c.g, c.b, 0);\n"
  "}\n"

a shader like the above causes problems, granted the webgl engines might block that sort of thing, but we should fix it properly!
Comment 10 fjhenigman 2013-05-22 15:02:47 UTC
This approach of failing to compile was rejected:
http://lists.freedesktop.org/archives/mesa-dev/2013-January/033659.html
I intended to write a patch taking that objection into account but haven't done so yet.
Comment 11 Dave Airlie 2013-06-06 22:05:13 UTC
fixed in master.


Use of freedesktop.org services, including Bugzilla, is subject to our Code of Conduct. How we collect and use information is described in our Privacy Policy.