From mboxrd@z Thu Jan 1 00:00:00 1970 From: Andrew Cooper Subject: Re: [PATCH 19/22] libxc: check failure of xc_dom_*_to_ptr, xc_map_foreign_range Date: Thu, 13 Jun 2013 16:41:23 +0100 Message-ID: <51B9E823.30407@citrix.com> References: <1370974960-19824-1-git-send-email-ian.jackson@eu.citrix.com> <1370974960-19824-20-git-send-email-ian.jackson@eu.citrix.com> <20921.58316.911595.30289@mariner.uk.xensource.com> Mime-Version: 1.0 Content-Type: text/plain; charset="us-ascii" Content-Transfer-Encoding: 7bit Return-path: In-Reply-To: <20921.58316.911595.30289@mariner.uk.xensource.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 13/06/13 16:22, Ian Jackson wrote: > Ian Jackson writes ("[PATCH 19/22] 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. > I seem to have missed one. I intend to add this to v8 of the series. > > commit 029d07d15fb27ab4b107c961b7ad74701ea623a8 > Author: Ian Jackson > Date: Thu Jun 13 16:19:58 2013 +0100 > > xc_exchange_page > > diff --git a/.topmsg b/.topmsg > index f211f1f..4ca5d4f 100644 > --- a/.topmsg > +++ b/.topmsg > @@ -12,6 +12,12 @@ This is part of the fix to a security issue, XSA-55. > Signed-off-by: Ian Jackson > Reviewed-by: Andrew Cooper Again, looks good. Feel free to carry forward my Reviewed-by tag. ~Andrew > > +v8: Add a missing check in xc_offline_page.c:xc_exchange_page. > + 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 > diff --git a/tools/libxc/xc_offline_page.c b/tools/libxc/xc_offline_page.c > index 089a361..36b9812 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 new_p for copy, guest may be broken?"); > + goto failed; > + } > memcpy(new_p, backup, PAGE_SIZE); > munmap(new_p, PAGE_SIZE); > mops.arg1.mfn = new_mfn;