[Date Prev][Date Next][Thread Prev][Thread Next][Date Index][Thread Index]

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



On 07/06/13 19:27, Ian Jackson wrote:
> 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.
>
> 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 this version of the series.
> ---
>  tools/libxc/xc_dom_armzimageloader.c |    6 ++++
>  tools/libxc/xc_dom_binloader.c       |    6 ++++
>  tools/libxc/xc_dom_core.c            |    6 ++++
>  tools/libxc/xc_dom_elfloader.c       |   13 ++++++++++
>  tools/libxc/xc_dom_x86.c             |   45 
> ++++++++++++++++++++++++++++++++++
>  tools/libxc/xc_domain_restore.c      |   27 ++++++++++++++++++++
>  6 files changed, 103 insertions(+), 0 deletions(-)
>
> diff --git a/tools/libxc/xc_dom_armzimageloader.c 
> b/tools/libxc/xc_dom_armzimageloader.c
> index 74027db..4cbbbab 100644
> --- a/tools/libxc/xc_dom_armzimageloader.c
> +++ b/tools/libxc/xc_dom_armzimageloader.c
> @@ -140,6 +140,12 @@ static int xc_dom_load_zimage_kernel(struct xc_dom_image 
> *dom)
>      DOMPRINTF_CALLED(dom->xch);
>  
>      dst = xc_dom_seg_to_ptr(dom, &dom->kernel_seg);
> +    if ( dst == NULL )
> +    {
> +        DOMPRINTF("%s: xc_dom_seg_to_ptr(dom, &dom->kernel_seg) => NULL",
> +                  __func__);
> +        return -1;
> +    }
>  
>      DOMPRINTF("%s: kernel sed %#"PRIx64"-%#"PRIx64,
>                __func__, dom->kernel_seg.vstart, dom->kernel_seg.vend);
> diff --git a/tools/libxc/xc_dom_binloader.c b/tools/libxc/xc_dom_binloader.c
> index 7eaf071..891bcea 100644
> --- a/tools/libxc/xc_dom_binloader.c
> +++ b/tools/libxc/xc_dom_binloader.c
> @@ -277,6 +277,12 @@ static int xc_dom_load_bin_kernel(struct xc_dom_image 
> *dom)
>      DOMPRINTF("  bss_size:  0x%" PRIx32 "", bss_size);
>  
>      dest = xc_dom_vaddr_to_ptr(dom, dom->kernel_seg.vstart, &dest_size);
> +    if ( dest == NULL )
> +    {
> +        DOMPRINTF("%s: xc_dom_vaddr_to_ptr(dom, dom->kernel_seg.vstart)"
> +                  " => NULL", __FUNCTION__);
> +        return -EINVAL;
> +    }

Consistency of __FUNCTION__ vs __func__ ?

It looks to be inconsistent across the codebase, but __FUNCTION__ is
nonstandard whereas __func__ is specified by C99.

>  
>      if ( dest_size < text_size ||
>           dest_size - text_size < bss_size )
> diff --git a/tools/libxc/xc_dom_core.c b/tools/libxc/xc_dom_core.c
> index cf96bfa..21a8e0d 100644
> --- a/tools/libxc/xc_dom_core.c
> +++ b/tools/libxc/xc_dom_core.c
> @@ -870,6 +870,12 @@ int xc_dom_build_image(struct xc_dom_image *dom)
>                                    ramdisklen) != 0 )
>              goto err;
>          ramdiskmap = xc_dom_seg_to_ptr(dom, &dom->ramdisk_seg);
> +        if ( ramdiskmap == NULL )
> +        {
> +            DOMPRINTF("%s: xc_dom_seg_to_ptr(dom, &dom->ramdisk_seg) => 
> NULL",
> +                      __FUNCTION__);
> +            goto err;
> +        }
>          if ( unziplen )
>          {
>              if ( xc_dom_do_gunzip(dom->xch,
> diff --git a/tools/libxc/xc_dom_elfloader.c b/tools/libxc/xc_dom_elfloader.c
> index 1d2727e..ac9bb3b 100644
> --- a/tools/libxc/xc_dom_elfloader.c
> +++ b/tools/libxc/xc_dom_elfloader.c
> @@ -137,6 +137,12 @@ static elf_errorstatus xc_dom_load_elf_symtab(struct 
> xc_dom_image *dom,
>              return 0;
>          size = dom->kernel_seg.vend - dom->bsd_symtab_start;
>          hdr_ptr = xc_dom_vaddr_to_ptr(dom, dom->bsd_symtab_start, 
> &allow_size);
> +        if ( hdr_ptr == NULL )
> +        {
> +            DOMPRINTF("%s/%s: xc_dom_vaddr_to_ptr(,dom->bsd_symtab_start"
> +                      " => NULL", __FUNCTION__, load ? "load" : "parse");
> +            return -1;
> +        }

Here, you are in an "if ( load )" block, so the conditional operator on
load is unnecessary.

There is however an xc_dom_malloc() in the associated else block lacking
any printing, which would line up with the "parse" half.

Also, consistency of error messages.  Previously you have had "(dom,
...)" instead of "(,"

>          elf->caller_xdest_base = hdr_ptr;
>          elf->caller_xdest_size = allow_size;
>          hdr = ELF_REALPTR2PTRVAL(hdr_ptr);
> @@ -383,6 +389,13 @@ static elf_errorstatus xc_dom_load_elf_kernel(struct 
> xc_dom_image *dom)
>  
>      elf->dest_base = xc_dom_seg_to_ptr_pages(dom, &dom->kernel_seg, &pages);
>      elf->dest_size = pages * XC_DOM_PAGE_SIZE(dom);
> +    if ( elf->dest_base == NULL )
> +    {
> +        DOMPRINTF("%s: xc_dom_vaddr_to_ptr(,dom->kernel_seg)"
> +                  " => NULL", __FUNCTION__);
> +        return -1;
> +    }
> +

You set elf->dest_size before checking for a NULL pointer on elf->dest_base.

As 'pages' will be 0 in the case of a failed xc_dom_seg_to_ptr_pages, it
should be safe, but should probably be fixed.

~Andrew

>      rc = elf_load_binary(elf);
>      if ( rc < 0 )
>      {
> diff --git a/tools/libxc/xc_dom_x86.c b/tools/libxc/xc_dom_x86.c
> index f1be43b..8b6191d 100644
> --- a/tools/libxc/xc_dom_x86.c
> +++ b/tools/libxc/xc_dom_x86.c
> @@ -223,6 +223,12 @@ static xen_pfn_t move_l3_below_4G(struct xc_dom_image 
> *dom,
>          goto out;
>  
>      l3tab = xc_dom_pfn_to_ptr(dom, l3pfn, 1);
> +    if ( l3tab == NULL )
> +    {
> +        DOMPRINTF("%s: xc_dom_pfn_to_ptr(dom, l3pfn, 1) => NULL",
> +                  __FUNCTION__);
> +        return l3mfn; /* our one call site will call xc_dom_panic and fail */
> +    }
>      memset(l3tab, 0, XC_DOM_PAGE_SIZE(dom));
>  
>      DOMPRINTF("%s: successfully relocated L3 below 4G. "
> @@ -266,6 +272,8 @@ static int setup_pgtables_x86_32_pae(struct xc_dom_image 
> *dom)
>      }
>  
>      l3tab = xc_dom_pfn_to_ptr(dom, l3pfn, 1);
> +    if ( l3tab == NULL )
> +        goto pfn_error;
>  
>      for ( addr = dom->parms.virt_base; addr < dom->virt_pgtab_end;
>            addr += PAGE_SIZE_X86 )
> @@ -274,6 +282,8 @@ static int setup_pgtables_x86_32_pae(struct xc_dom_image 
> *dom)
>          {
>              /* get L2 tab, make L3 entry */
>              l2tab = xc_dom_pfn_to_ptr(dom, l2pfn, 1);
> +            if ( l2tab == NULL )
> +                goto pfn_error;
>              l3off = l3_table_offset_pae(addr);
>              l3tab[l3off] =
>                  pfn_to_paddr(xc_dom_p2m_guest(dom, l2pfn)) | L3_PROT;
> @@ -284,6 +294,8 @@ static int setup_pgtables_x86_32_pae(struct xc_dom_image 
> *dom)
>          {
>              /* get L1 tab, make L2 entry */
>              l1tab = xc_dom_pfn_to_ptr(dom, l1pfn, 1);
> +            if ( l1tab == NULL )
> +                goto pfn_error;
>              l2off = l2_table_offset_pae(addr);
>              l2tab[l2off] =
>                  pfn_to_paddr(xc_dom_p2m_guest(dom, l1pfn)) | L2_PROT;
> @@ -310,6 +322,11 @@ static int setup_pgtables_x86_32_pae(struct xc_dom_image 
> *dom)
>          l3tab[3] = pfn_to_paddr(xc_dom_p2m_guest(dom, l2pfn)) | L3_PROT;
>      }
>      return 0;
> +
> +pfn_error:
> +    xc_dom_panic(dom->xch, XC_INTERNAL_ERROR,
> +                 "%s: xc_dom_pfn_to_ptr failed", __FUNCTION__);
> +    return -EINVAL;
>  }
>  
>  #undef L1_PROT
> @@ -347,6 +364,9 @@ static int setup_pgtables_x86_64(struct xc_dom_image *dom)
>      uint64_t addr;
>      xen_pfn_t pgpfn;
>  
> +    if ( l4tab == NULL )
> +        goto pfn_error;
> +
>      for ( addr = dom->parms.virt_base; addr < dom->virt_pgtab_end;
>            addr += PAGE_SIZE_X86 )
>      {
> @@ -354,6 +374,8 @@ static int setup_pgtables_x86_64(struct xc_dom_image *dom)
>          {
>              /* get L3 tab, make L4 entry */
>              l3tab = xc_dom_pfn_to_ptr(dom, l3pfn, 1);
> +            if ( l3tab == NULL )
> +                goto pfn_error;
>              l4off = l4_table_offset_x86_64(addr);
>              l4tab[l4off] =
>                  pfn_to_paddr(xc_dom_p2m_guest(dom, l3pfn)) | L4_PROT;
> @@ -364,6 +386,8 @@ static int setup_pgtables_x86_64(struct xc_dom_image *dom)
>          {
>              /* get L2 tab, make L3 entry */
>              l2tab = xc_dom_pfn_to_ptr(dom, l2pfn, 1);
> +            if ( l2tab == NULL )
> +                goto pfn_error;
>              l3off = l3_table_offset_x86_64(addr);
>              l3tab[l3off] =
>                  pfn_to_paddr(xc_dom_p2m_guest(dom, l2pfn)) | L3_PROT;
> @@ -376,6 +400,8 @@ static int setup_pgtables_x86_64(struct xc_dom_image *dom)
>          {
>              /* get L1 tab, make L2 entry */
>              l1tab = xc_dom_pfn_to_ptr(dom, l1pfn, 1);
> +            if ( l1tab == NULL )
> +                goto pfn_error;
>              l2off = l2_table_offset_x86_64(addr);
>              l2tab[l2off] =
>                  pfn_to_paddr(xc_dom_p2m_guest(dom, l1pfn)) | L2_PROT;
> @@ -396,6 +422,11 @@ static int setup_pgtables_x86_64(struct xc_dom_image 
> *dom)
>              l1tab = NULL;
>      }
>      return 0;
> +
> +pfn_error:
> +    xc_dom_panic(dom->xch, XC_INTERNAL_ERROR,
> +                 "%s: xc_dom_pfn_to_ptr failed", __FUNCTION__);
> +    return -EINVAL;
>  }
>  
>  #undef L1_PROT
> @@ -413,6 +444,8 @@ static int alloc_magic_pages(struct xc_dom_image *dom)
>      if ( xc_dom_alloc_segment(dom, &dom->p2m_seg, "phys2mach", 0, p2m_size) )
>          return -1;
>      dom->p2m_guest = xc_dom_seg_to_ptr(dom, &dom->p2m_seg);
> +    if ( dom->p2m_guest == NULL )
> +        return -1;
>  
>      /* allocate special pages */
>      dom->start_info_pfn = xc_dom_alloc_page(dom, "start info");
> @@ -437,6 +470,12 @@ static int start_info_x86_32(struct xc_dom_image *dom)
>  
>      DOMPRINTF_CALLED(dom->xch);
>  
> +    if ( start_info == NULL )
> +    {
> +        DOMPRINTF("%s: xc_dom_pfn_to_ptr failed on start_info", 
> __FUNCTION__);
> +        return -1; /* our caller throws away our return value :-/ */
> +    }
> +
>      memset(start_info, 0, sizeof(*start_info));
>      strncpy(start_info->magic, dom->guest_type, sizeof(start_info->magic));
>      start_info->magic[sizeof(start_info->magic) - 1] = '\0';
> @@ -477,6 +516,12 @@ static int start_info_x86_64(struct xc_dom_image *dom)
>  
>      DOMPRINTF_CALLED(dom->xch);
>  
> +    if ( start_info == NULL )
> +    {
> +        DOMPRINTF("%s: xc_dom_pfn_to_ptr failed on start_info", 
> __FUNCTION__);
> +        return -1; /* our caller throws away our return value :-/ */
> +    }
> +
>      memset(start_info, 0, sizeof(*start_info));
>      strncpy(start_info->magic, dom->guest_type, sizeof(start_info->magic));
>      start_info->magic[sizeof(start_info->magic) - 1] = '\0';
> diff --git a/tools/libxc/xc_domain_restore.c b/tools/libxc/xc_domain_restore.c
> index a15f86a..c7835ff 100644
> --- a/tools/libxc/xc_domain_restore.c
> +++ b/tools/libxc/xc_domain_restore.c
> @@ -1638,6 +1638,12 @@ int xc_domain_restore(xc_interface *xch, int io_fd, 
> uint32_t dom,
>                      mfn = ctx->p2m[pfn];
>                      buf = xc_map_foreign_range(xch, dom, PAGE_SIZE,
>                                                 PROT_READ | PROT_WRITE, mfn);
> +                    if ( buf == NULL )
> +                    {
> +                        ERROR("xc_map_foreign_range for generation id"
> +                              " buffer failed");
> +                        goto out;
> +                    }
>  
>                      generationid = *(unsigned long long *)(buf + offset);
>                      *(unsigned long long *)(buf + offset) = generationid + 1;
> @@ -1794,6 +1800,11 @@ int xc_domain_restore(xc_interface *xch, int io_fd, 
> uint32_t dom,
>                  l3tab = (uint64_t *)
>                      xc_map_foreign_range(xch, dom, PAGE_SIZE,
>                                           PROT_READ, ctx->p2m[i]);
> +                if ( l3tab == NULL )
> +                {
> +                    PERROR("xc_map_foreign_range failed (for l3tab)");
> +                    goto out;
> +                }
>  
>                  for ( j = 0; j < 4; j++ )
>                      l3ptes[j] = l3tab[j];
> @@ -1820,6 +1831,11 @@ int xc_domain_restore(xc_interface *xch, int io_fd, 
> uint32_t dom,
>                  l3tab = (uint64_t *)
>                      xc_map_foreign_range(xch, dom, PAGE_SIZE,
>                                           PROT_READ | PROT_WRITE, 
> ctx->p2m[i]);
> +                if ( l3tab == NULL )
> +                {
> +                    PERROR("xc_map_foreign_range failed (for l3tab, 2nd)");
> +                    goto out;
> +                }
>  
>                  for ( j = 0; j < 4; j++ )
>                      l3tab[j] = l3ptes[j];
> @@ -1996,6 +2012,12 @@ int xc_domain_restore(xc_interface *xch, int io_fd, 
> uint32_t dom,
>              SET_FIELD(ctxt, user_regs.edx, mfn);
>              start_info = xc_map_foreign_range(
>                  xch, dom, PAGE_SIZE, PROT_READ | PROT_WRITE, mfn);
> +            if ( start_info == NULL )
> +            {
> +                PERROR("xc_map_foreign_range failed (for start_info)");
> +                goto out;
> +            }
> +
>              SET_FIELD(start_info, nr_pages, dinfo->p2m_size);
>              SET_FIELD(start_info, shared_info, 
> shared_info_frame<<PAGE_SHIFT);
>              SET_FIELD(start_info, flags, 0);
> @@ -2143,6 +2165,11 @@ int xc_domain_restore(xc_interface *xch, int io_fd, 
> uint32_t dom,
>      /* Restore contents of shared-info page. No checking needed. */
>      new_shared_info = xc_map_foreign_range(
>          xch, dom, PAGE_SIZE, PROT_WRITE, shared_info_frame);
> +    if ( new_shared_info == NULL )
> +    {
> +        PERROR("xc_map_foreign_range failed (for new_shared_info)");
> +        goto out;
> +    }
>  
>      /* restore saved vcpu_info and arch specific info */
>      MEMCPY_FIELD(new_shared_info, old_shared_info, vcpu_info);


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


 


Rackspace

Lists.xenproject.org is hosted with RackSpace, monitoring our
servers 24x7x365 and backed by RackSpace's Fanatical Support®.