[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:24, Ian Jackson wrote:
George Dunlap writes ("Re: [Xen-devel] [PATCH 18/23] libxc: Add range checking to 
xc_dom_binloader"):
I think disturbing it in a way that is obviously correct is better than
disturbing it in a way that looks redundant.
That's an argument.

How about this then ?
(diff against previous tip followed by revised patch)

Ian.

commit 202da02f40b1806138419002c3e29dc8ce0e88c2
Author: Ian Jackson <ian.jackson@xxxxxxxxxxxxx>
Date:   Fri Jun 14 16:23:44 2013 +0100

     Use clearer code for calculating probe_end

diff --git a/.topmsg b/.topmsg
index c5cff24..1e1b932 100644
--- a/.topmsg
+++ b/.topmsg
@@ -25,6 +25,8 @@ This is part of the fix to a security issue, XSA-55.
  Signed-off-by: Ian Jackson <ian.jackson@xxxxxxxxxxxxx>
  Reviewed-by: Andrew Cooper <andrew.cooper3@xxxxxxxxxx>
+v9: Use clearer code for calculating probe_end in find_table.
+
  v6: Add a missing `return -EINVAL' (Matthew Daley).
      Fix an error in the commit message (Matthew Daley).
diff --git a/tools/libxc/xc_dom_binloader.c b/tools/libxc/xc_dom_binloader.c
index 7eaf071..6469a65 100644
--- a/tools/libxc/xc_dom_binloader.c
+++ b/tools/libxc/xc_dom_binloader.c
@@ -126,10 +126,10 @@ static struct xen_bin_image_table *find_table(struct 
xc_dom_image *dom)
      if ( dom->kernel_size < sizeof(*table) )
          return NULL;
      probe_ptr = dom->kernel_blob;
-    probe_end = dom->kernel_blob + dom->kernel_size - sizeof(*table);
-    if ( dom->kernel_size >= 8192 &&
-         (void*)probe_end > (dom->kernel_blob + 8192) )
+    if ( dom->kernel_size > (8192 + sizeof(*table)) )
          probe_end = dom->kernel_blob + 8192;
+    else
+        probe_end = dom->kernel_blob + dom->kernel_size - sizeof(*table);
for ( table = NULL; probe_ptr < probe_end; probe_ptr++ )
      {



From: Ian Jackson <ian.jackson@xxxxxxxxxxxxx>
Subject: [PATCH] libxc: Add range checking to xc_dom_binloader

This is a simple binary image loader with its own metadata format.
However, it is too careless with image-supplied values.

Add the following checks:

  * That the image is bigger than the metadata table; otherwise the
    pointer arithmetic to calculate the metadata table location may
    yield undefined and dangerous values.

  * 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.

  * That the supplied image is big enough for the text we are allegedly
    copying from it.  Otherwise we might have a read overrun and copy
    the results (perhaps a lot of secret data) into the guest.

This is part of the fix to a security issue, XSA-55.

Signed-off-by: Ian Jackson <ian.jackson@xxxxxxxxxxxxx>
Reviewed-by: Andrew Cooper <andrew.cooper3@xxxxxxxxxx>

This looks good -- thanks:

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


_______________________________________________
Xen-devel mailing list
Xen-devel@xxxxxxxxxxxxx
http://lists.xen.org/xen-devel


 


Rackspace

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