[Mesa-dev] [PATCH] i965: fix problem with constant out of bounds access

[Dave Airlie]

On Thu, May 30, 2013 at 10:21 AM, Eric Anholt <eric at anholt.net> wrote:
> Dave Airlie <airlied at gmail.com> writes:
>
>> From: Dave Airlie <airlied at 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 <airlied at 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.