Re: [Xen-devel] [PATCH 03/22] libxc: Fix range checking in xc_dom_pfn_to_ptr etc.

[Andrew Cooper <andrew.cooper3@xxxxxxxxxx>]

On 07/06/13 19:27, Ian Jackson wrote:
> * Ensure that xc_dom_pfn_to_ptr (when called with count==0) does not
>   return a previously-allocated block which is entirely before the
>   requested pfn (!)
>
> * Provide a version of xc_dom_pfn_to_ptr, xc_dom_pfn_to_ptr_retcount,
>   which provides the length of the mapped region via an out parameter.
>
> * Change xc_dom_vaddr_to_ptr to always provide the length of the
>   mapped region and change the call site in xc_dom_binloader.c to
>   check it.  The call site in xc_dom_load_elf_symtab will be corrected
>   in a forthcoming patch, and for now ignores the returned length.
>
> This is part of the fix to a security issue, XSA-55.
>
> Signed-off-by: Ian Jackson <ian.jackson@xxxxxxxxxxxxx>
>
> v5: This patch is new in v5 of the series.
> ---
>  tools/libxc/xc_dom.h           |   16 +++++++++++++---
>  tools/libxc/xc_dom_binloader.c |   11 ++++++++++-
>  tools/libxc/xc_dom_core.c      |   13 +++++++++++++
>  tools/libxc/xc_dom_elfloader.c |    3 ++-
>  4 files changed, 38 insertions(+), 5 deletions(-)
>
> diff --git a/tools/libxc/xc_dom.h b/tools/libxc/xc_dom.h
> index da0598c..c7eaf85 100644
> --- a/tools/libxc/xc_dom.h
> +++ b/tools/libxc/xc_dom.h
> @@ -291,6 +291,8 @@ int xc_dom_alloc_segment(struct xc_dom_image *dom,
>  
>  void *xc_dom_pfn_to_ptr(struct xc_dom_image *dom, xen_pfn_t first,
>                          xen_pfn_t count);
> +void *xc_dom_pfn_to_ptr_retcount(struct xc_dom_image *dom, xen_pfn_t first,
> +                                 xen_pfn_t count, xen_pfn_t *count_out);
>  void xc_dom_unmap_one(struct xc_dom_image *dom, xen_pfn_t pfn);
>  void xc_dom_unmap_all(struct xc_dom_image *dom);
>  
> @@ -317,13 +319,21 @@ static inline void *xc_dom_seg_to_ptr(struct 
> xc_dom_image *dom,
>  }
>  
>  static inline void *xc_dom_vaddr_to_ptr(struct xc_dom_image *dom,
> -                                        xen_vaddr_t vaddr)
> +                                        xen_vaddr_t vaddr,
> +                                        size_t *safe_region_out)
>  {
>      unsigned int page_size = XC_DOM_PAGE_SIZE(dom);
>      xen_pfn_t page = (vaddr - dom->parms.virt_base) / page_size;
>      unsigned int offset = (vaddr - dom->parms.virt_base) % page_size;
> -    void *ptr = xc_dom_pfn_to_ptr(dom, page, 0);
> -    return (ptr ? (ptr + offset) : NULL);
> +    xen_pfn_t safe_region_count;
> +    void *ptr;
> +
> +    *safe_region_out = 0;

In PATCH 2, the pages_out parameter is optional in so far as it is
checked for being NULL.

Here, the safe_region_out parameter is not optional, in so far as we
unconditionally fault if it is NULL. (unless someone is playing games
and mapping something at address 0)

Is it worth having something such as

if ( ! safe_region_out )
  return NULL;

partly for consistency and party on grounds of sanity.  As far as I can
tell, this function is not private to libxc.

~Andrew

> +    ptr = xc_dom_pfn_to_ptr_retcount(dom, page, 0, &safe_region_count);
> +    if ( ptr == NULL )
> +        return ptr;
> +    *safe_region_out = (safe_region_count << XC_DOM_PAGE_SHIFT(dom)) - 
> offset;
> +    return ptr;
>  }
>  
>  static inline xen_pfn_t xc_dom_p2m_host(struct xc_dom_image *dom, xen_pfn_t 
> pfn)
> diff --git a/tools/libxc/xc_dom_binloader.c b/tools/libxc/xc_dom_binloader.c
> index c14727c..d2de04c 100644
> --- a/tools/libxc/xc_dom_binloader.c
> +++ b/tools/libxc/xc_dom_binloader.c
> @@ -249,6 +249,7 @@ static int xc_dom_load_bin_kernel(struct xc_dom_image 
> *dom)
>      char *image = dom->kernel_blob;
>      char *dest;
>      size_t image_size = dom->kernel_size;
> +    size_t dest_size;
>      uint32_t start_addr;
>      uint32_t load_end_addr;
>      uint32_t bss_end_addr;
> @@ -272,7 +273,15 @@ static int xc_dom_load_bin_kernel(struct xc_dom_image 
> *dom)
>      DOMPRINTF("  text_size: 0x%" PRIx32 "", text_size);
>      DOMPRINTF("  bss_size:  0x%" PRIx32 "", bss_size);
>  
> -    dest = xc_dom_vaddr_to_ptr(dom, dom->kernel_seg.vstart);
> +    dest = xc_dom_vaddr_to_ptr(dom, dom->kernel_seg.vstart, &dest_size);
> +
> +    if ( dest_size < text_size ||
> +         dest_size - text_size < bss_size )
> +    {
> +        DOMPRINTF("%s: mapped region is too small for image", __FUNCTION__);
> +        return -EINVAL;
> +    }
> +
>      memcpy(dest, image + skip, text_size);
>      memset(dest + text_size, 0, bss_size);
>  
> diff --git a/tools/libxc/xc_dom_core.c b/tools/libxc/xc_dom_core.c
> index b92e4a9..cf96bfa 100644
> --- a/tools/libxc/xc_dom_core.c
> +++ b/tools/libxc/xc_dom_core.c
> @@ -351,11 +351,20 @@ int xc_dom_try_gunzip(struct xc_dom_image *dom, void 
> **blob, size_t * size)
>  void *xc_dom_pfn_to_ptr(struct xc_dom_image *dom, xen_pfn_t pfn,
>                          xen_pfn_t count)
>  {
> +    xen_pfn_t count_out_dummy;
> +    return xc_dom_pfn_to_ptr_retcount(dom, pfn, count, &count_out_dummy);
> +}
> +
> +void *xc_dom_pfn_to_ptr_retcount(struct xc_dom_image *dom, xen_pfn_t pfn,
> +                                 xen_pfn_t count, xen_pfn_t *count_out)
> +{
>      struct xc_dom_phys *phys;
>      xen_pfn_t offset;
>      unsigned int page_shift = XC_DOM_PAGE_SHIFT(dom);
>      char *mode = "unset";
>  
> +    *count_out = 0;
> +
>      offset = pfn - dom->rambase_pfn;
>      if ( offset > dom->total_pages || /* multiple checks to avoid overflows 
> */
>           count > dom->total_pages ||
> @@ -386,6 +395,7 @@ void *xc_dom_pfn_to_ptr(struct xc_dom_image *dom, 
> xen_pfn_t pfn,
>                            phys->count);
>                  return NULL;
>              }
> +            *count_out = count;
>          }
>          else
>          {
> @@ -393,6 +403,9 @@ void *xc_dom_pfn_to_ptr(struct xc_dom_image *dom, 
> xen_pfn_t pfn,
>                 just hand out a pointer to it */
>              if ( pfn < phys->first )
>                  continue;
> +            if ( pfn >= phys->first + phys->count )
> +                continue;
> +            *count_out = phys->count - (pfn - phys->first);
>          }
>          return phys->ptr + ((pfn - phys->first) << page_shift);
>      }
> diff --git a/tools/libxc/xc_dom_elfloader.c b/tools/libxc/xc_dom_elfloader.c
> index 6583859..bc92302 100644
> --- a/tools/libxc/xc_dom_elfloader.c
> +++ b/tools/libxc/xc_dom_elfloader.c
> @@ -128,10 +128,11 @@ static int xc_dom_load_elf_symtab(struct xc_dom_image 
> *dom,
>  
>      if ( load )
>      {
> +        size_t allow_size; /* will be used in a forthcoming XSA-55 patch */
>          if ( !dom->bsd_symtab_start )
>              return 0;
>          size = dom->kernel_seg.vend - dom->bsd_symtab_start;
> -        hdr  = xc_dom_vaddr_to_ptr(dom, dom->bsd_symtab_start);
> +        hdr  = xc_dom_vaddr_to_ptr(dom, dom->bsd_symtab_start, &allow_size);
>          *(int *)hdr = size - sizeof(int);
>      }
>      else


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