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

[George Dunlap <george.dunlap@eu.citrix.com>]

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@eu.citrix.com>
> 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@eu.citrix.com>
>   Reviewed-by: Andrew Cooper <andrew.cooper3@citrix.com>
>   
> +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@eu.citrix.com>
> 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@eu.citrix.com>
> Reviewed-by: Andrew Cooper <andrew.cooper3@citrix.com>

This looks good -- thanks:

Reviewed-by: George Dunlap <george.dunlap@eu.citrix.com>