Re: [PATCH 19/22] libxc: check failure of xc_dom_*_to_ptr, xc_map_foreign_range

[Andrew Cooper <andrew.cooper3@citrix.com>]

On 13/06/13 16:22, Ian Jackson wrote:
> Ian Jackson writes ("[PATCH 19/22] libxc: check failure of xc_dom_*_to_ptr, xc_map_foreign_range"):
>> The return values from xc_dom_*_to_ptr and xc_map_foreign_range are
>> sometimes dereferenced, or subjected to pointer arithmetic, without
>> checking whether the relevant function failed and returned NULL.
>>
>> Add an appropriate error check at every call site.
> I seem to have missed one.  I intend to add this to v8 of the series.
>
> commit 029d07d15fb27ab4b107c961b7ad74701ea623a8
> Author: Ian Jackson <ian.jackson@eu.citrix.com>
> Date:   Thu Jun 13 16:19:58 2013 +0100
>
>     xc_exchange_page
>
> diff --git a/.topmsg b/.topmsg
> index f211f1f..4ca5d4f 100644
> --- a/.topmsg
> +++ b/.topmsg
> @@ -12,6 +12,12 @@ 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>

Again, looks good.  Feel free to carry forward my Reviewed-by tag.

~Andrew

>  
> +v8: Add a missing check in xc_offline_page.c:xc_exchange_page.
> +     I think in this particular error case it may be that the results
> +     are a broken guest, but turning this from a possible host tools
> +     crash into a guest problem seems to solve the potential security
> +     problem.
> +
>  v7: Simplify an error DOMPRINTF to not use "load ? : ".
>      Make DOMPRINTF allocation error messages consistent.
>      Do not set elf->dest_pages in xc_dom_load_elf_kernel
> diff --git a/tools/libxc/xc_offline_page.c b/tools/libxc/xc_offline_page.c
> index 089a361..36b9812 100644
> --- a/tools/libxc/xc_offline_page.c
> +++ b/tools/libxc/xc_offline_page.c
> @@ -714,6 +714,11 @@ int xc_exchange_page(xc_interface *xch, int domid, xen_pfn_t mfn)
>  
>          new_p = xc_map_foreign_range(xch, domid, PAGE_SIZE,
>                                       PROT_READ|PROT_WRITE, new_mfn);
> +        if ( new_p == NULL )
> +        {
> +            ERROR("failed to map new_p for copy, guest may be broken?");
> +            goto failed;
> +        }
>          memcpy(new_p, backup, PAGE_SIZE);
>          munmap(new_p, PAGE_SIZE);
>          mops.arg1.mfn = new_mfn;