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

[Ian Romanick]

On 05/29/2013 04:54 PM, Dave Airlie wrote:
> 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).

I didn't know Frank was going to disappear of the face of the Earth 
after my patch feed back. :(  I thought he was still working on it, so I 
didn't pick it up.

> 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..
>
> CVE-2013-1872
>
> Signed-off-by: Dave Airlie <airlied at redhat.com>

Reviewed-by: Ian Romanick <ian.d.romanick at intel.com>

One suggestion below...

> ---
>   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.
> +             */

I'd either replace or augment this with justification from the GLSL spec:

     /* Section 5.11 of the OpenGL 4.3 spec says:
      *
      *     "Out-of-bounds reads return undefined values, which include
      *     values from other variables of the active program or zero."
      */

> +	    if (constant_nr < 0 || constant_nr >= (int)c->prog_data.nr_params) {
> +               constant_nr = 0;
> +	    }
>
>   	    /* For now, set this to non-negative.  We'll give it the
>   	     * actual new number in a moment, in order to keep the
> @@ -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;
>   }
>
>   fs_visitor::~fs_visitor()
>