Re: [PATCH 02/22] libxc: introduce xc_dom_seg_to_ptr_pages

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

On 07/06/13 19:27, Ian Jackson wrote:
> Provide a version of xc_dom_seg_to_ptr which returns the number of
> guest pages it has actually mapped.  This is useful for callers who
> want to do range checking; we will use this later in this series.
>
> This is part of the fix to a security issue, XSA-55.
>
> Signed-off-by: Ian Jackson <ian.jackson@eu.citrix.com>
>
> v5: xc_dom_seg_to_ptr_pages sets *pages_out=0 if it returns NULL.
>
> v4 was:
>
> Acked-by: Ian Campbell <ian.campbell@citrix.com>
> Reviewed-by: Konrad Rzeszutek Wilk <konrad.wilk@oracle.com>

With a view to security, why does this change need to be present?

At the end of the v6 series, this new function has 1 caller from
xc_dom_load_elf_kernel() and 5 callers of the original function.

>From looking at the semantics of the function, it either returns NULL,
or maps all requested pages (xc_dom_seg size rounded up to the nearest page)

With that in mind, xc_dom_load_elf_kernel() can calculate elf->dest_size
given a successful mapping from xc_dom_seg_to_ptr(), without needing
this _pages() variant.

As a result, I do not think it is needed to put this change in, unless I
am missing something about the semantics.

~Andrew

> ---
>  tools/libxc/xc_dom.h |   18 +++++++++++++++---
>  1 files changed, 15 insertions(+), 3 deletions(-)
>
> diff --git a/tools/libxc/xc_dom.h b/tools/libxc/xc_dom.h
> index ac36600..da0598c 100644
> --- a/tools/libxc/xc_dom.h
> +++ b/tools/libxc/xc_dom.h
> @@ -294,14 +294,26 @@ void *xc_dom_pfn_to_ptr(struct xc_dom_image *dom, xen_pfn_t first,
>  void xc_dom_unmap_one(struct xc_dom_image *dom, xen_pfn_t pfn);
>  void xc_dom_unmap_all(struct xc_dom_image *dom);
>  
> -static inline void *xc_dom_seg_to_ptr(struct xc_dom_image *dom,
> -                                      struct xc_dom_seg *seg)
> +static inline void *xc_dom_seg_to_ptr_pages(struct xc_dom_image *dom,
> +                                      struct xc_dom_seg *seg,
> +                                      xen_pfn_t *pages_out)
>  {
>      xen_vaddr_t segsize = seg->vend - seg->vstart;
>      unsigned int page_size = XC_DOM_PAGE_SIZE(dom);
>      xen_pfn_t pages = (segsize + page_size - 1) / page_size;
> +    void *retval;
> +
> +    retval = xc_dom_pfn_to_ptr(dom, seg->pfn, pages);
> +
> +    if ( pages_out != NULL )
> +        *pages_out = retval ? pages : 0;
> +    return retval;
> +}
>  
> -    return xc_dom_pfn_to_ptr(dom, seg->pfn, pages);
> +static inline void *xc_dom_seg_to_ptr(struct xc_dom_image *dom,
> +                                      struct xc_dom_seg *seg)
> +{
> +    return xc_dom_seg_to_ptr_pages(dom, seg, NULL);
>  }
>  
>  static inline void *xc_dom_vaddr_to_ptr(struct xc_dom_image *dom,