From mboxrd@z Thu Jan 1 00:00:00 1970 From: Andrew Cooper Subject: Re: [PATCH 20/22] libxc: check return values from malloc Date: Tue, 11 Jun 2013 20:05:45 +0100 Message-ID: <51B77509.5040909@citrix.com> References: <1370974865-19554-1-git-send-email-ian.jackson@eu.citrix.com> <1370974865-19554-21-git-send-email-ian.jackson@eu.citrix.com> Mime-Version: 1.0 Content-Type: text/plain; charset="us-ascii" Content-Transfer-Encoding: 7bit Return-path: In-Reply-To: <1370974865-19554-21-git-send-email-ian.jackson@eu.citrix.com> List-Unsubscribe: , List-Post: List-Help: List-Subscribe: , Sender: xen-devel-bounces@lists.xen.org Errors-To: xen-devel-bounces@lists.xen.org To: Ian Jackson Cc: "xen-devel@lists.xensource.com" , "mattjd@gmail.com" , "security@xen.org" List-Id: xen-devel@lists.xenproject.org On 11/06/13 19:21, Ian Jackson wrote: > A sufficiently malformed input to libxc (such as a malformed input ELF > or other guest-controlled data) might cause one of libxc's malloc() to > fail. In this case we need to make sure we don't dereference or do > pointer arithmetic on the result. > > Search for all occurrences of \b(m|c|re)alloc in libxc, and all > functions which call them, and add appropriate error checking where > missing. > > This includes the functions xc_dom_malloc*, which now print a message > when they fail so that callers don't have to do so. > > The function xc_cpuid_to_str wasn't provided with a sane return value > and has a pretty strange API, which now becomes a little stranger. > There are no in-tree callers. > > This is part of the fix to a security issue, XSA-55. > > Signed-off-by: Ian Jackson Reviewed-by: Andrew Cooper > > v7: Add a missing check for a call to alloc_str. > Add arithmetic overflow check in xc_dom_malloc. > Coding style fix. > > v6: Fix a missed call `pfn_err = calloc...' in xc_domain_restore.c. > Fix a missed call `new_pfn = xc_map_foreign_range...' in > xc_offline_page.c > > v5: This patch is new in this version of the series. > --- > tools/libxc/xc_cpuid_x86.c | 20 ++++++++++++++++++-- > tools/libxc/xc_dom_arm.c | 2 ++ > tools/libxc/xc_dom_core.c | 13 +++++++++++++ > tools/libxc/xc_dom_elfloader.c | 2 ++ > tools/libxc/xc_dom_x86.c | 3 +++ > tools/libxc/xc_domain_restore.c | 13 +++++++++++++ > tools/libxc/xc_linux_osdep.c | 4 ++++ > tools/libxc/xc_offline_page.c | 5 +++++ > tools/libxc/xc_private.c | 2 ++ > tools/libxc/xenctrl.h | 2 +- > 10 files changed, 63 insertions(+), 3 deletions(-) > > diff --git a/tools/libxc/xc_cpuid_x86.c b/tools/libxc/xc_cpuid_x86.c > index 17efc0f..fa47787 100644 > --- a/tools/libxc/xc_cpuid_x86.c > +++ b/tools/libxc/xc_cpuid_x86.c > @@ -590,6 +590,8 @@ static int xc_cpuid_do_domctl( > static char *alloc_str(void) > { > char *s = malloc(33); > + if ( s == NULL ) > + return s; > memset(s, 0, 33); > return s; > } > @@ -601,6 +603,8 @@ void xc_cpuid_to_str(const unsigned int *regs, char **strs) > for ( i = 0; i < 4; i++ ) > { > strs[i] = alloc_str(); > + if ( strs[i] == NULL ) > + continue; > for ( j = 0; j < 32; j++ ) > strs[i][j] = !!((regs[i] & (1U << (31 - j)))) ? '1' : '0'; > } > @@ -681,7 +685,7 @@ int xc_cpuid_check( > const char **config, > char **config_transformed) > { > - int i, j; > + int i, j, rc; > unsigned int regs[4]; > > memset(config_transformed, 0, 4 * sizeof(*config_transformed)); > @@ -693,6 +697,11 @@ int xc_cpuid_check( > if ( config[i] == NULL ) > continue; > config_transformed[i] = alloc_str(); > + if ( config_transformed[i] == NULL ) > + { > + rc = -ENOMEM; > + goto fail_rc; > + } > for ( j = 0; j < 32; j++ ) > { > unsigned char val = !!((regs[i] & (1U << (31 - j)))); > @@ -709,12 +718,14 @@ int xc_cpuid_check( > return 0; > > fail: > + rc = -EPERM; > + fail_rc: > for ( i = 0; i < 4; i++ ) > { > free(config_transformed[i]); > config_transformed[i] = NULL; > } > - return -EPERM; > + return rc; > } > > /* > @@ -759,6 +770,11 @@ int xc_cpuid_set( > } > > config_transformed[i] = alloc_str(); > + if ( config_transformed[i] == NULL ) > + { > + rc = -ENOMEM; > + goto fail; > + } > > for ( j = 0; j < 32; j++ ) > { > diff --git a/tools/libxc/xc_dom_arm.c b/tools/libxc/xc_dom_arm.c > index aaf35ca..df59ffb 100644 > --- a/tools/libxc/xc_dom_arm.c > +++ b/tools/libxc/xc_dom_arm.c > @@ -170,6 +170,8 @@ int arch_setup_meminit(struct xc_dom_image *dom) > dom->shadow_enabled = 1; > > dom->p2m_host = xc_dom_malloc(dom, sizeof(xen_pfn_t) * dom->total_pages); > + if ( dom->p2m_host == NULL ) > + return -EINVAL; > > /* setup initial p2m */ > for ( pfn = 0; pfn < dom->total_pages; pfn++ ) > diff --git a/tools/libxc/xc_dom_core.c b/tools/libxc/xc_dom_core.c > index 21a8e0d..1a14d3c 100644 > --- a/tools/libxc/xc_dom_core.c > +++ b/tools/libxc/xc_dom_core.c > @@ -120,9 +120,17 @@ void *xc_dom_malloc(struct xc_dom_image *dom, size_t size) > { > struct xc_dom_mem *block; > > + if ( size > SIZE_MAX - sizeof(*block) ) > + { > + DOMPRINTF("%s: unreasonable allocation size", __FUNCTION__); > + return NULL; > + } > block = malloc(sizeof(*block) + size); > if ( block == NULL ) > + { > + DOMPRINTF("%s: allocation failed", __FUNCTION__); > return NULL; > + } > memset(block, 0, sizeof(*block) + size); > block->next = dom->memblocks; > dom->memblocks = block; > @@ -138,7 +146,10 @@ void *xc_dom_malloc_page_aligned(struct xc_dom_image *dom, size_t size) > > block = malloc(sizeof(*block)); > if ( block == NULL ) > + { > + DOMPRINTF("%s: allocation failed", __FUNCTION__); > return NULL; > + } > memset(block, 0, sizeof(*block)); > block->mmap_len = size; > block->mmap_ptr = mmap(NULL, block->mmap_len, > @@ -146,6 +157,7 @@ void *xc_dom_malloc_page_aligned(struct xc_dom_image *dom, size_t size) > -1, 0); > if ( block->mmap_ptr == MAP_FAILED ) > { > + DOMPRINTF("%s: mmap failed", __FUNCTION__); > free(block); > return NULL; > } > @@ -202,6 +214,7 @@ void *xc_dom_malloc_filemap(struct xc_dom_image *dom, > close(fd); > if ( block != NULL ) > free(block); > + DOMPRINTF("%s: failed (on file `%s')", __FUNCTION__, filename); > return NULL; > } > > diff --git a/tools/libxc/xc_dom_elfloader.c b/tools/libxc/xc_dom_elfloader.c > index 7563f18..5246a03 100644 > --- a/tools/libxc/xc_dom_elfloader.c > +++ b/tools/libxc/xc_dom_elfloader.c > @@ -327,6 +327,8 @@ static elf_errorstatus xc_dom_parse_elf_kernel(struct xc_dom_image *dom) > return rc; > > elf = xc_dom_malloc(dom, sizeof(*elf)); > + if ( elf == NULL ) > + return -1; > dom->private_loader = elf; > rc = elf_init(elf, dom->kernel_blob, dom->kernel_size); > xc_elf_set_logfile(dom->xch, elf, 1); > diff --git a/tools/libxc/xc_dom_x86.c b/tools/libxc/xc_dom_x86.c > index 8b6191d..126c0f8 100644 > --- a/tools/libxc/xc_dom_x86.c > +++ b/tools/libxc/xc_dom_x86.c > @@ -760,6 +760,9 @@ int arch_setup_meminit(struct xc_dom_image *dom) > } > > dom->p2m_host = xc_dom_malloc(dom, sizeof(xen_pfn_t) * dom->total_pages); > + if ( dom->p2m_host == NULL ) > + return -EINVAL; > + > if ( dom->superpages ) > { > int count = dom->total_pages >> SUPERPAGE_PFN_SHIFT; > diff --git a/tools/libxc/xc_domain_restore.c b/tools/libxc/xc_domain_restore.c > index c7835ff..f53ff88 100644 > --- a/tools/libxc/xc_domain_restore.c > +++ b/tools/libxc/xc_domain_restore.c > @@ -1243,6 +1243,11 @@ static int apply_batch(xc_interface *xch, uint32_t dom, struct restore_ctx *ctx, > > /* Map relevant mfns */ > pfn_err = calloc(j, sizeof(*pfn_err)); > + if ( pfn_err == NULL ) > + { > + PERROR("allocation for pfn_err failed"); > + return -1; > + } > region_base = xc_map_foreign_bulk( > xch, dom, PROT_WRITE, region_mfn, pfn_err, j); > > @@ -1532,8 +1537,16 @@ int xc_domain_restore(xc_interface *xch, int io_fd, uint32_t dom, > region_mfn = malloc(ROUNDUP(MAX_BATCH_SIZE * sizeof(xen_pfn_t), PAGE_SHIFT)); > ctx->p2m_batch = malloc(ROUNDUP(MAX_BATCH_SIZE * sizeof(xen_pfn_t), PAGE_SHIFT)); > if (!ctx->hvm && ctx->superpages) > + { > ctx->p2m_saved_batch = > malloc(ROUNDUP(MAX_BATCH_SIZE * sizeof(xen_pfn_t), PAGE_SHIFT)); > + if ( ctx->p2m_saved_batch == NULL ) > + { > + ERROR("saved batch memory alloc failed"); > + errno = ENOMEM; > + goto out; > + } > + } > > if ( (ctx->p2m == NULL) || (pfn_type == NULL) || > (region_mfn == NULL) || (ctx->p2m_batch == NULL) ) > diff --git a/tools/libxc/xc_linux_osdep.c b/tools/libxc/xc_linux_osdep.c > index 36832b6..73860a2 100644 > --- a/tools/libxc/xc_linux_osdep.c > +++ b/tools/libxc/xc_linux_osdep.c > @@ -378,6 +378,8 @@ static void *linux_privcmd_map_foreign_range(xc_interface *xch, xc_osdep_handle > > num = (size + XC_PAGE_SIZE - 1) >> XC_PAGE_SHIFT; > arr = calloc(num, sizeof(xen_pfn_t)); > + if ( arr == NULL ) > + return NULL; > > for ( i = 0; i < num; i++ ) > arr[i] = mfn + i; > @@ -402,6 +404,8 @@ static void *linux_privcmd_map_foreign_ranges(xc_interface *xch, xc_osdep_handle > num_per_entry = chunksize >> XC_PAGE_SHIFT; > num = num_per_entry * nentries; > arr = calloc(num, sizeof(xen_pfn_t)); > + if ( arr == NULL ) > + return NULL; > > for ( i = 0; i < nentries; i++ ) > for ( j = 0; j < num_per_entry; j++ ) > diff --git a/tools/libxc/xc_offline_page.c b/tools/libxc/xc_offline_page.c > index 089a361..5aad79d 100644 > --- a/tools/libxc/xc_offline_page.c > +++ b/tools/libxc/xc_offline_page.c > @@ -714,6 +714,11 @@ int xc_exchange_page(xc_interface *xch, int domid, xen_pfn_t mfn) > > new_p = xc_map_foreign_range(xch, domid, PAGE_SIZE, > PROT_READ|PROT_WRITE, new_mfn); > + if ( new_p == NULL ) > + { > + ERROR("failed to map foreign range for new_p"); > + goto failed; > + } > memcpy(new_p, backup, PAGE_SIZE); > munmap(new_p, PAGE_SIZE); > mops.arg1.mfn = new_mfn; > diff --git a/tools/libxc/xc_private.c b/tools/libxc/xc_private.c > index e891cc8..acaf9e0 100644 > --- a/tools/libxc/xc_private.c > +++ b/tools/libxc/xc_private.c > @@ -771,6 +771,8 @@ const char *xc_strerror(xc_interface *xch, int errcode) > errbuf = pthread_getspecific(errbuf_pkey); > if (errbuf == NULL) { > errbuf = malloc(XS_BUFSIZE); > + if ( errbuf == NULL ) > + return "(failed to allocate errbuf)"; > pthread_setspecific(errbuf_pkey, errbuf); > } > > diff --git a/tools/libxc/xenctrl.h b/tools/libxc/xenctrl.h > index c024af4..f6f4ceb 100644 > --- a/tools/libxc/xenctrl.h > +++ b/tools/libxc/xenctrl.h > @@ -1829,7 +1829,7 @@ int xc_cpuid_set(xc_interface *xch, > int xc_cpuid_apply_policy(xc_interface *xch, > domid_t domid); > void xc_cpuid_to_str(const unsigned int *regs, > - char **strs); > + char **strs); /* some strs[] may be NULL if ENOMEM */ > int xc_mca_op(xc_interface *xch, struct xen_mc *mc); > #endif >