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

[Ian Jackson <Ian.Jackson@xxxxxxxxxxxxx>]

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>

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

v5: This patch is new in this version of the series.

---
 tools/libxc/xc_dom_binloader.c |   15 +++++++++++++--
 1 files changed, 13 insertions(+), 2 deletions(-)

diff --git a/tools/libxc/xc_dom_binloader.c b/tools/libxc/xc_dom_binloader.c
index d2de04c..6469a65 100644
--- a/tools/libxc/xc_dom_binloader.c
+++ b/tools/libxc/xc_dom_binloader.c
@@ -123,10 +123,13 @@ static struct xen_bin_image_table *find_table(struct 
xc_dom_image *dom)
     uint32_t *probe_ptr;
     uint32_t *probe_end;
 
+    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 + 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++ )
     {
@@ -282,6 +285,14 @@ static int xc_dom_load_bin_kernel(struct xc_dom_image *dom)
         return -EINVAL;
     }
 
+    if ( image_size < skip ||
+         image_size - skip < text_size )
+    {
+        DOMPRINTF("%s: image is too small for declared text size",
+                  __FUNCTION__);
+        return -EINVAL;
+    }
+
     memcpy(dest, image + skip, text_size);
     memset(dest + text_size, 0, bss_size);
 
-- 
tg: (91d5c0e..) xsa55/binloader (depends on: xsa55/unobsolete)

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