<html>
<head>
<meta content="text/html; charset=ISO-8859-1"
http-equiv="Content-Type">
</head>
<body bgcolor="#FFFFFF" text="#000000">
<div class="moz-cite-prefix">On 01/14/2014 01:24 AM, Paul Berry
wrote:<br>
</div>
<blockquote
cite="mid:CA+yLL67ZkRV4Pc6vrnY5TGtSZwwGqqv3uRqEwtAMoWpeeA=dMQ@mail.gmail.com"
type="cite">
<meta http-equiv="Content-Type" content="text/html;
charset=ISO-8859-1">
<div dir="ltr">On 2 January 2014 03:58, Tapani Pälli <span
dir="ltr"><<a moz-do-not-send="true"
href="mailto:tapani.palli@intel.com" target="_blank">tapani.palli@intel.com</a>></span>
wrote:<br>
<div class="gmail_extra">
<div class="gmail_quote">
<blockquote class="gmail_quote" style="margin:0 0 0
.8ex;border-left:1px #ccc solid;padding-left:1ex">+<br>
+<br>
+/**<br>
+ * Reads header part of the binary blob. Main purpose of
this header is to<br>
+ * validate that cached shader was produced with same
Mesa driver version.<br>
+ */<br>
+int<br>
+ir_deserializer::read_header(struct gl_shader *shader)<br>
+{<br>
+ char *cache_magic_id = map->read_string();<br>
+ char *driver_vendor = map->read_string();<br>
+ char *driver_renderer = map->read_string();<br>
+<br>
+ /* only used or debug output, silence compiler warning
*/<br>
+ (void) driver_vendor;<br>
+ (void) driver_renderer;<br>
</blockquote>
<div><br>
</div>
<div>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. <br>
</div>
<div> </div>
</div>
</div>
</div>
</blockquote>
<br>
ok, I'll add check against this<br>
<br>
<blockquote
cite="mid:CA+yLL67ZkRV4Pc6vrnY5TGtSZwwGqqv3uRqEwtAMoWpeeA=dMQ@mail.gmail.com"
type="cite">
<div dir="ltr">
<div class="gmail_extra">
<div class="gmail_quote">
<blockquote class="gmail_quote" style="margin:0 0 0
.8ex;border-left:1px #ccc solid;padding-left:1ex">
+<br>
+ shader->Version = map->read_uint32_t();<br>
+ shader->Type = map->read_uint32_t();<br>
+ shader->IsES = map->read_uint8_t();<br>
+<br>
+ CACHE_DEBUG("%s: version %d, type 0x%x, %s (mesa
%s)\n[%s %s]\n",<br>
+ __func__, shader->Version,
shader->Type,<br>
+ (shader->IsES) ? "glsl es" : "desktop
glsl",<br>
+ cache_magic_id, driver_vendor,
driver_renderer);<br>
+<br>
+ const char *magic = mesa_get_shader_cache_magic();<br>
+<br>
+ if (memcmp(cache_magic_id, magic, strlen(magic)))<br>
+ return DIFFERENT_MESA_VER;<br>
</blockquote>
<div><br>
</div>
<div>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().<br>
</div>
<div> </div>
</div>
</div>
</div>
</blockquote>
<br>
will fix<br>
<br>
<blockquote
cite="mid:CA+yLL67ZkRV4Pc6vrnY5TGtSZwwGqqv3uRqEwtAMoWpeeA=dMQ@mail.gmail.com"
type="cite">
<div dir="ltr">
<div class="gmail_extra">
<div class="gmail_quote">
<blockquote class="gmail_quote" style="margin:0 0 0
.8ex;border-left:1px #ccc solid;padding-left:1ex">
+ for (unsigned k = 0; k < length; k++) {<br>
+ uint8_t row_major, interpolation, centroid;<br>
+ int32_t location;<br>
+ char *field_name = map->read_string();<br>
+ fields[k].name = _mesa_strdup(field_name);<br>
+ fields[k].type = read_glsl_type();<br>
+ row_major = map->read_uint8_t();<br>
+ location = map->read_int32_t();<br>
+ interpolation = map->read_uint8_t();<br>
+ centroid = map->read_uint8_t();<br>
+ fields[k].row_major = row_major;<br>
+ fields[k].location = location;<br>
+ fields[k].interpolation = interpolation;<br>
+ fields[k].centroid = centroid;<br>
</blockquote>
<div><br>
</div>
<div>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.<br>
</div>
</div>
</div>
</div>
</blockquote>
<br>
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.<br>
<br>
<br>
<blockquote
cite="mid:CA+yLL67ZkRV4Pc6vrnY5TGtSZwwGqqv3uRqEwtAMoWpeeA=dMQ@mail.gmail.com"
type="cite">
<div dir="ltr">
<div class="gmail_extra">
<div class="gmail_quote">
<blockquote class="gmail_quote" style="margin:0 0 0
.8ex;border-left:1px #ccc solid;padding-left:1ex">
+ var->state_slots = NULL;<br>
+<br>
+ if (var->num_state_slots > 0) {<br>
+ var->state_slots = ralloc_array(var,
ir_state_slot,<br>
+ var->num_state_slots);<br>
</blockquote>
<div><br>
</div>
<div>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.<br>
</div>
</div>
</div>
</div>
</blockquote>
<br>
What is the expected reasonable range for state_slots?<br>
<br>
<blockquote
cite="mid:CA+yLL67ZkRV4Pc6vrnY5TGtSZwwGqqv3uRqEwtAMoWpeeA=dMQ@mail.gmail.com"
type="cite">
<div dir="ltr">
<div class="gmail_extra">
<div class="gmail_quote">
<blockquote class="gmail_quote" style="margin:0 0 0
.8ex;border-left:1px #ccc solid;padding-left:1ex">
+ /* used for debugging */<br>
+ (void) ir_type;<br>
+ (void) len;<br>
</blockquote>
<div><br>
</div>
<div>This is a nice opportunity to do some validation and
detect corrupted blobs. I think we should go ahead and do
that.<br>
</div>
</div>
</div>
</div>
</blockquote>
<br>
OK, will add validation of read ir_type against expected ir_type for
all the rvalues.<br>
<br>
<blockquote
cite="mid:CA+yLL67ZkRV4Pc6vrnY5TGtSZwwGqqv3uRqEwtAMoWpeeA=dMQ@mail.gmail.com"
type="cite">
<div dir="ltr">
<div class="gmail_extra">
<div class="gmail_quote">
<div> </div>
<blockquote class="gmail_quote" style="margin:0 0 0
.8ex;border-left:1px #ccc solid;padding-left:1ex">
+ /* peek type of next instruction */<br>
</blockquote>
<div><br>
</div>
<div>"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?<br>
</div>
</div>
</div>
</div>
</blockquote>
<br>
Code is correct, s/peek/switch/<br>
<br>
<blockquote
cite="mid:CA+yLL67ZkRV4Pc6vrnY5TGtSZwwGqqv3uRqEwtAMoWpeeA=dMQ@mail.gmail.com"
type="cite">
<div dir="ltr">
<div class="gmail_extra">
<div class="gmail_quote">
<blockquote class="gmail_quote" style="margin:0 0 0
.8ex;border-left:1px #ccc solid;padding-left:1ex">
+ /* fill parse state from shader header information */<br>
+ switch (shader->Type) {<br>
+ case GL_VERTEX_SHADER:<br>
+ state->target = MESA_SHADER_VERTEX;<br>
+ break;<br>
+ case GL_FRAGMENT_SHADER:<br>
+ state->target = MESA_SHADER_FRAGMENT;<br>
+ break;<br>
+ case GL_GEOMETRY_SHADER_ARB:<br>
+ state->target = MESA_SHADER_GEOMETRY;<br>
+ break;<br>
</blockquote>
<div><br>
</div>
<div>There should be a default case here to handle corrupted
blobs with an illegal type.<br>
</div>
</div>
</div>
</div>
</blockquote>
<br>
I'm now doing _mesa_shader_enum_to_shader_stage(shader->Type)
here which will assert if type is illegal.<br>
<br>
Thanks for going through this;<br>
<br>
// Tapani<br>
</body>
</html>