From aaebaba5ae225f591e0602e071037a935bb281b6 Mon Sep 17 00:00:00 2001 From: Ian Jackson Date: Fri, 14 Jun 2013 16:39:38 +0100 Subject: 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. This is part of the fix to a security issue, XSA-55. Signed-off-by: Ian Jackson Reviewed-by: Andrew Cooper v8: Add a missing check in xc_offline_page.c:xc_exchange_page, which was in the next patch in v7 of the series. Also improve the message. 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 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 ++++++++++++++++++++++ tools/libxc/xc_offline_page.c | 5 ++++ 7 files changed, 108 insertions(+) (limited to 'tools/libxc') diff --git a/tools/libxc/xc_dom_armzimageloader.c b/tools/libxc/xc_dom_armzimageloader.c index 74027dbe34..4cbbbab16f 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 6469a651cc..e1de5b5c21 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 cf96bfa99b..21a8e0deb6 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 f2bc2f597e..8d0a09f752 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 f1be43bed6..8b6191d25b 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 a15f86a787..c7835ff6a9 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<