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

[Eric Anholt]

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

> @@ -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?

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.
-------------- next part --------------
A non-text attachment was scrubbed...
Name: not available
Type: application/pgp-signature
Size: 197 bytes
Desc: not available
URL: <http://lists.freedesktop.org/archives/mesa-dev/attachments/20130529/ca883abe/attachment.pgp>