From: Andrew Cooper <andrew.cooper3@citrix.com>
To: Ian Jackson <ian.jackson@eu.citrix.com>
Cc: "xen-devel@lists.xensource.com" <xen-devel@lists.xensource.com>,
"mattjd@gmail.com" <mattjd@gmail.com>,
"security@xen.org" <security@xen.org>
Subject: Re: [PATCH 19/22] libxc: check failure of xc_dom_*_to_ptr, xc_map_foreign_range
Date: Tue, 11 Jun 2013 20:10:59 +0100 [thread overview]
Message-ID: <51B77643.4070500@citrix.com> (raw)
In-Reply-To: <1370974865-19554-20-git-send-email-ian.jackson@eu.citrix.com>
On 11/06/13 19:21, 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@eu.citrix.com>
Reviewed-by: Andrew Cooper <andrew.cooper3@citrix.com>
>
> 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
> if xc_dom_seg_to_ptr_pages fails.
>
> 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;
> + }
>
> 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..7563f18 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/load: xc_dom_vaddr_to_ptr(dom,dom->bsd_symtab_start"
> + " => NULL", __FUNCTION__);
> + return -1;
> + }
> elf->caller_xdest_base = hdr_ptr;
> elf->caller_xdest_size = allow_size;
> hdr = ELF_REALPTR2PTRVAL(hdr_ptr);
> @@ -382,7 +388,14 @@ static elf_errorstatus xc_dom_load_elf_kernel(struct xc_dom_image *dom)
> xen_pfn_t pages;
>
> elf->dest_base = xc_dom_seg_to_ptr_pages(dom, &dom->kernel_seg, &pages);
> + if ( elf->dest_base == NULL )
> + {
> + DOMPRINTF("%s: xc_dom_vaddr_to_ptr(dom,dom->kernel_seg)"
> + " => NULL", __FUNCTION__);
> + return -1;
> + }
> elf->dest_size = pages * XC_DOM_PAGE_SIZE(dom);
> +
> 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);
next prev parent reply other threads:[~2013-06-11 19:10 UTC|newest]
Thread overview: 51+ messages / expand[flat|nested] mbox.gz Atom feed top
2013-06-11 18:20 [PATCH v7 00/22] XSA55 libelf fixes for unstable Ian Jackson
2013-06-11 18:20 ` [PATCH 01/22] libelf: abolish libelf-relocate.c Ian Jackson
2013-06-11 18:20 ` [PATCH 02/22] libxc: introduce xc_dom_seg_to_ptr_pages Ian Jackson
2013-06-11 18:44 ` Andrew Cooper
2013-06-11 18:20 ` [PATCH 03/22] libxc: Fix range checking in xc_dom_pfn_to_ptr etc Ian Jackson
2013-06-11 19:01 ` Andrew Cooper
2013-06-11 18:20 ` [PATCH 04/22] libelf: add `struct elf_binary*' parameter to elf_load_image Ian Jackson
2013-06-11 18:20 ` [PATCH 05/22] libelf: abolish elf_sval and elf_access_signed Ian Jackson
2013-06-11 18:20 ` [PATCH 06/22] libelf: move include of <asm/guest_access.h> to top of file Ian Jackson
2013-06-11 18:20 ` [PATCH 07/22] libelf/xc_dom_load_elf_symtab: Do not use "syms" uninitialised Ian Jackson
2013-06-11 18:20 ` [PATCH 08/22] libelf: introduce macros for memory access and pointer handling Ian Jackson
2013-06-11 18:20 ` [PATCH 09/22] tools/xcutils/readnotes: adjust print_l1_mfn_valid_note Ian Jackson
2013-06-11 18:20 ` [PATCH 10/22] libelf: check nul-terminated strings properly Ian Jackson
2013-06-11 19:14 ` Andrew Cooper
2013-06-11 18:20 ` [PATCH 11/22] libelf: check all pointer accesses Ian Jackson
2013-06-11 19:19 ` Andrew Cooper
2013-06-11 18:20 ` [PATCH 12/22] libelf: Check pointer references in elf_is_elfbinary Ian Jackson
2013-06-11 19:03 ` Andrew Cooper
2013-06-11 18:20 ` [PATCH 13/22] libelf: Make all callers call elf_check_broken Ian Jackson
2013-06-11 18:20 ` [PATCH 14/22] libelf: use C99 bool for booleans Ian Jackson
2013-06-11 19:04 ` Andrew Cooper
2013-06-11 18:20 ` [PATCH 15/22] libelf: use only unsigned integers Ian Jackson
2013-06-11 19:22 ` Andrew Cooper
2013-06-11 18:20 ` [PATCH 16/22] libelf: check loops for running away Ian Jackson
2013-06-11 19:28 ` Andrew Cooper
2013-06-11 18:21 ` [PATCH 17/22] libelf: abolish obsolete macros Ian Jackson
2013-06-11 18:21 ` [PATCH 18/22] libxc: Add range checking to xc_dom_binloader Ian Jackson
2013-06-11 19:11 ` Andrew Cooper
2013-06-11 18:21 ` [PATCH 19/22] libxc: check failure of xc_dom_*_to_ptr, xc_map_foreign_range Ian Jackson
2013-06-11 19:10 ` Andrew Cooper [this message]
2013-06-11 18:21 ` [PATCH 20/22] libxc: check return values from malloc Ian Jackson
2013-06-11 19:05 ` Andrew Cooper
2013-06-11 18:21 ` [PATCH 21/22] libxc: range checks in xc_dom_p2m_host and _guest Ian Jackson
2013-06-11 19:06 ` Andrew Cooper
2013-06-12 16:02 ` George Dunlap
2013-06-12 16:06 ` Ian Jackson
2013-06-12 16:08 ` George Dunlap
2013-06-12 18:26 ` Tim Deegan
2013-06-11 18:21 ` [PATCH 22/22] libxc: check blob size before proceeding in xc_dom_check_gzip Ian Jackson
2013-06-11 19:08 ` Andrew Cooper
2013-06-11 19:30 ` [PATCH v7 00/22] XSA55 libelf fixes for unstable Andrew Cooper
2013-06-12 13:45 ` Ian Jackson
2013-06-12 14:02 ` Ian Jackson
2013-06-12 14:19 ` Andrew Cooper
-- strict thread matches above, loose matches on Subject: below --
2013-06-11 18:22 [PATCH v7 00/22] XSA55 libelf fixes for Xen 4.2 Ian Jackson
2013-06-11 18:22 ` [PATCH 19/22] libxc: check failure of xc_dom_*_to_ptr, xc_map_foreign_range Ian Jackson
2013-06-13 15:22 ` Ian Jackson
2013-06-13 15:41 ` Andrew Cooper
2013-06-07 18:35 [PATCH v6 00/22] XSA55 libelf fixes for Xen 4.2 Ian Jackson
2013-06-07 18:35 ` [PATCH 19/22] libxc: check failure of xc_dom_*_to_ptr, xc_map_foreign_range Ian Jackson
2013-06-07 18:27 [PATCH v6 00/22] XSA55 libelf fixes for unstable Ian Jackson
2013-06-07 18:27 ` [PATCH 19/22] libxc: check failure of xc_dom_*_to_ptr, xc_map_foreign_range Ian Jackson
2013-06-10 21:09 ` Andrew Cooper
2013-06-11 14:44 ` Ian Jackson
Reply instructions:
You may reply publicly to this message via plain-text email
using any one of the following methods:
* Save the following mbox file, import it into your mail client,
and reply-to-all from there: mbox
Avoid top-posting and favor interleaved quoting:
https://en.wikipedia.org/wiki/Posting_style#Interleaved_style
* Reply using the --to, --cc, and --in-reply-to
switches of git-send-email(1):
git send-email \
--in-reply-to=51B77643.4070500@citrix.com \
--to=andrew.cooper3@citrix.com \
--cc=ian.jackson@eu.citrix.com \
--cc=mattjd@gmail.com \
--cc=security@xen.org \
--cc=xen-devel@lists.xensource.com \
/path/to/YOUR_REPLY
https://kernel.org/pub/software/scm/git/docs/git-send-email.html
* If your mail client supports setting the In-Reply-To header
via mailto: links, try the mailto: link
Be sure your reply has a Subject: header at the top and a blank line
before the message body.
This is a public inbox, see mirroring instructions
for how to clone and mirror all data and code used for this inbox;
as well as URLs for NNTP newsgroup(s).