[Date Prev][Date Next][Thread Prev][Thread Next][Date Index][Thread Index]

Re: [Xen-devel] [PATCH 18/23] libxc: Add range checking to xc_dom_binloader

On 14/06/13 16:03, Ian Jackson wrote:
George Dunlap writes ("Re: [Xen-devel] [PATCH 18/23] libxc: Add range checking to 
On Thu, Jun 13, 2013 at 7:14 PM, Ian Jackson <ian.jackson@xxxxxxxxxxxxx> wrote:
  * When clamping the end of the region to search, that we do not
    calculate pointers beyond the end of the image.  The C
    specification does not permit this and compilers are becoming ever
    more determined to miscompile code when they can "prove" various
    falsehoods based on assertions from the C spec.
+    if ( dom->kernel_size < sizeof(*table) )
+        return NULL;
      probe_ptr = dom->kernel_blob;
      probe_end = dom->kernel_blob + dom->kernel_size - sizeof(*table);
-    if ( (void*)probe_end > (dom->kernel_blob + 8192) )
+    if ( dom->kernel_size >= 8192 &&
+         (void*)probe_end > (dom->kernel_blob + 8192) )
          probe_end = dom->kernel_blob + 8192;
Wait, what's going on here?  Isn't the point of this check originally
that "probe_end" might be pointing off into nowhere, and you're going
to "clip" it into pointing somewhere reasonable?
No.  The (undocumented) format being parsed here is that the info
table should start no later than 8k into the file.  probe_end is the
first address to no longer look for the table at.

Firstly we set probe_end to the end of the image minus the table
length - which would imply searching the whole image.  The (original
and unchanged) purpose of the if statement and assignment is to limit
this to 8192 bytes from the start of the image.

However there is a bug: if the image is less than 8k long, this
involves computing a wild pointer (which is forbidden).  So in my
patch I add an additional test.

It doesn't look like you've actually changed any pointer arithmetic --
if either probe_end or dom->kernel_blob + 8192 are wild, then they'll
still be wild after this check, won't they?
The initial value of probe_end is guaranteed not to be wild.

Before my change  dom->kernel_blob + 8192  might be computed even if
it is wild; after my change it is only computed if it is guaranteed
not to be.

Oh, I see -- right; so if "dom->kernel_size < 8192", then "dom->kernel_blob+8192" might be wild; but as long as "dom->kernel_size > 8192", then "dom->kernel_blob + 8192" will be fine.

And if dom->kernel_size < 8192, then probe_end will already be < dom->kernel_blob+8192, so we don't need to clip things.

Might be nice to put a comment here, so people coming later can make sense out of this. Even if people read the changeset description, they may, like me, not be able to suss out which calculation is in danger of being wild.

Other than that:

Reviewed-by: George Dunlap <george.dunlap@xxxxxxxxxxxxx>

Xen-devel mailing list



Lists.xenproject.org is hosted with RackSpace, monitoring our
servers 24x7x365 and backed by RackSpace's Fanatical Support®.