[Mesa-dev] [wip 6/9] glsl: ir_deserializer class for the binary shader cache

[Tapani Pälli]

On 01/14/2014 01:24 AM, Paul Berry wrote:
> On 2 January 2014 03:58, Tapani Pälli <tapani.palli at intel.com 
> <mailto:tapani.palli at intel.com>> wrote:
>
>     +
>     +
>     +/**
>     + * Reads header part of the binary blob. Main purpose of this
>     header is to
>     + * validate that cached shader was produced with same Mesa driver
>     version.
>     + */
>     +int
>     +ir_deserializer::read_header(struct gl_shader *shader)
>     +{
>     +   char *cache_magic_id = map->read_string();
>     +   char *driver_vendor = map->read_string();
>     +   char *driver_renderer = map->read_string();
>     +
>     +   /* only used or debug output, silence compiler warning */
>     +   (void) driver_vendor;
>     +   (void) driver_renderer;
>
>
> A single version of Mesa potentially supports many different hardware 
> types, and those different hardware types may define different values 
> of GLSL built-in constants.  They also may require core Mesa to do 
> different sets of lowering passes during compilation.  So we can't 
> just ignore driver_vendor and driver_renderer. We need to reject the 
> binary blob if they don't match.

ok, I'll add check against this

>     +
>     +   shader->Version = map->read_uint32_t();
>     +   shader->Type = map->read_uint32_t();
>     +   shader->IsES = map->read_uint8_t();
>     +
>     +   CACHE_DEBUG("%s: version %d, type 0x%x, %s (mesa %s)\n[%s %s]\n",
>     +               __func__,  shader->Version, shader->Type,
>     +               (shader->IsES) ? "glsl es" : "desktop glsl",
>     +               cache_magic_id, driver_vendor, driver_renderer);
>     +
>     +   const char *magic = mesa_get_shader_cache_magic();
>     +
>     +   if (memcmp(cache_magic_id, magic, strlen(magic)))
>     +      return DIFFERENT_MESA_VER;
>
>
> If cache_magic_id is "foobar" and magic is "foo", this will erroneusly 
> consider them equal.  The correct way to do this is to use strcmp().

will fix

>     +      for (unsigned k = 0; k < length; k++) {
>     +         uint8_t row_major, interpolation, centroid;
>     +         int32_t location;
>     +         char *field_name = map->read_string();
>     +         fields[k].name = _mesa_strdup(field_name);
>     +         fields[k].type = read_glsl_type();
>     +         row_major = map->read_uint8_t();
>     +         location = map->read_int32_t();
>     +         interpolation = map->read_uint8_t();
>     +         centroid = map->read_uint8_t();
>     +         fields[k].row_major = row_major;
>     +         fields[k].location = location;
>     +         fields[k].interpolation = interpolation;
>     +         fields[k].centroid = centroid;
>
>
> Another security issue: if the binary blob is corrupted, length may be 
> outrageously large (e.g. 0xffffffff).  We need a way for this loop to 
> bail out and exit if it tries to read past the end of the binary blob.

I've added check now for this and other such cases where we might read 
past the end that you found. Check is is memory_map and these loops will 
ask if there were errors.


>     +   var->state_slots = NULL;
>     +
>     +   if (var->num_state_slots > 0) {
>     +      var->state_slots = ralloc_array(var, ir_state_slot,
>     +         var->num_state_slots);
>
>
> Security issue: if the shader blob is corrupted, num_state_slots could 
> be outrageously large, causing this memory allocation to fail (which 
> would then cause the loop below to perform illegal memory accesses).  
> We should validate that num_state_slots is within a reasonable range, 
> and if it isn't, abort reading the binary blob.

What is the expected reasonable range for state_slots?

>     +      /* used for debugging */
>     +      (void) ir_type;
>     +      (void) len;
>
>
> This is a nice opportunity to do some validation and detect corrupted 
> blobs.  I think we should go ahead and do that.

OK, will add validation of read ir_type against expected ir_type for all 
the rvalues.

>     +      /* peek type of next instruction */
>
>
> "peek" implies that you're going to look at the data but not advance 
> the read pointer.  But it looks like you're advancing the read pointer 
> anyhow.  Which is correct, the code or the comment?

Code is correct, s/peek/switch/

>     +   /* fill parse state from shader header information */
>     +   switch (shader->Type) {
>     +   case GL_VERTEX_SHADER:
>     +      state->target = MESA_SHADER_VERTEX;
>     +      break;
>     +   case GL_FRAGMENT_SHADER:
>     +      state->target = MESA_SHADER_FRAGMENT;
>     +      break;
>     +   case GL_GEOMETRY_SHADER_ARB:
>     +      state->target = MESA_SHADER_GEOMETRY;
>     +      break;
>
>
> There should be a default case here to handle corrupted blobs with an 
> illegal type.

I'm now doing _mesa_shader_enum_to_shader_stage(shader->Type) here which 
will assert if type is illegal.

Thanks for going through this;

// Tapani
-------------- next part --------------
An HTML attachment was scrubbed...
URL: <http://lists.freedesktop.org/archives/mesa-dev/attachments/20140114/6945fb9f/attachment.html>