On 01/14/2014 01:24 AM, Paul Berry wrote:
On 2 January 2014 03:58, Tapani Pälli <tapani.pa...@intel.com <mailto:tapani.pa...@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
_______________________________________________
mesa-dev mailing list
mesa-dev@lists.freedesktop.org
http://lists.freedesktop.org/mailman/listinfo/mesa-dev

Reply via email to