On Thu, May 30, 2013 at 10:21 AM, Eric Anholt <e...@anholt.net> wrote:
> Dave Airlie <airl...@gmail.com> writes:
>
>> From: Dave Airlie <airl...@redhat.com>
>>
>> This is my attempt at fixing this as the CVE is making RH security team
>> care enough to make me look at this. (please upstream, security fixes are
>> more important than whatever else you are doing, if for no other reason than
>> it saves me having to fix stuff I've no real clue about).
>>
>> Since Frank's original fix was denied, here is my attempt to just
>> alias all constants that are out of bounds < 0 or > nr_params to constant 0,
>> hopefully this provides the undefined behaviour idr requires..
>
>> Signed-off-by: Dave Airlie <airl...@redhat.com>
>> ---
>> src/mesa/drivers/dri/i965/brw_fs.cpp | 12 +++++++++++-
>> src/mesa/drivers/dri/i965/brw_fs_visitor.cpp | 1 +
>> 2 files changed, 12 insertions(+), 1 deletion(-)
>>
>> diff --git a/src/mesa/drivers/dri/i965/brw_fs.cpp
>> b/src/mesa/drivers/dri/i965/brw_fs.cpp
>> index baaa25c..62dbe71 100644
>> --- a/src/mesa/drivers/dri/i965/brw_fs.cpp
>> +++ b/src/mesa/drivers/dri/i965/brw_fs.cpp
>> @@ -1504,7 +1504,13 @@ fs_visitor::remove_dead_constants()
>> if (inst->src[i].file != UNIFORM)
>> continue;
>>
>> - assert(constant_nr < (int)c->prog_data.nr_params);
>> + /* if we get a negative constant nr or one greater than we can
>> + * handle, this can cause an overflow, we can't just refuse to
>> + * build, so just go undefined and alias everyone to constant 0.
>> + */
>> + if (constant_nr < 0 || constant_nr >= (int)c->prog_data.nr_params)
>> {
>> + constant_nr = 0;
>> + }
>
> bad indentation
Oops will fix it up to look like the rest.
>
>> @@ -1552,6 +1558,10 @@ fs_visitor::remove_dead_constants()
>> if (inst->src[i].file != UNIFORM)
>> continue;
>>
>> + /* as above alias to 0 */
>> + if (constant_nr < 0 || constant_nr >= (int)c->prog_data.nr_params)
>> {
>> + constant_nr = 0;
>> + }
>> assert(this->params_remap[constant_nr] != -1);
>> inst->src[i].reg = this->params_remap[constant_nr];
>> inst->src[i].reg_offset = 0;
>> diff --git a/src/mesa/drivers/dri/i965/brw_fs_visitor.cpp
>> b/src/mesa/drivers/dri/i965/brw_fs_visitor.cpp
>> index 36d9cf0..07c8088 100644
>> --- a/src/mesa/drivers/dri/i965/brw_fs_visitor.cpp
>> +++ b/src/mesa/drivers/dri/i965/brw_fs_visitor.cpp
>> @@ -2467,6 +2467,7 @@ fs_visitor::fs_visitor(struct brw_context *brw,
>> this->force_sechalf_stack = 0;
>>
>> memset(&this->param_size, 0, sizeof(this->param_size));
>> + this->params_remap = NULL;
>> }
>
> What's up with this last hunk?
Frank has this separated out from his fix, I think its required
alright, not sure why things
don't fall over now.
"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."
what a quote from him.
> This is definitely the right approach. On master we'd be doing our
> users a service to use _mesa_gl_debug() to tell people that they've done
> something wrong, but that would inhibit backporting so let's go with the
> rest of it as is.
Yeah we could do that in a follow on alright.
I'll fix the indents, and resend with just the first hunk, and we can
commit Frank's first patch separately, and backport both.
Dave.
_______________________________________________
mesa-dev mailing list
mesa-dev@lists.freedesktop.org
http://lists.freedesktop.org/mailman/listinfo/mesa-dev
