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

[Andrew Cooper <andrew.cooper3@xxxxxxxxxx>]

On 07/06/13 19:27, Ian Jackson wrote:
> 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>
>
> 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 |   13 ++++++++++++-
>  1 files changed, 12 insertions(+), 1 deletions(-)
>
> diff --git a/tools/libxc/xc_dom_binloader.c b/tools/libxc/xc_dom_binloader.c
> index d2de04c..7eaf071 100644
> --- a/tools/libxc/xc_dom_binloader.c
> +++ b/tools/libxc/xc_dom_binloader.c
> @@ -123,9 +123,12 @@ 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 &&
> +         (void*)probe_end > (dom->kernel_blob + 8192) )
>          probe_end = dom->kernel_blob + 8192;

More void pointer arithmetic.

If I am reading the above correctly, it appears to be a glorified

probe_end = dom->kernel_blob + (uintptr_t)min(dom->kernel_size -
sizeof(*table), 8192);

which looks to be a more simple representation?

~Andrew

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


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