From mboxrd@z Thu Jan 1 00:00:00 1970 From: Ian Jackson Subject: Re: [PATCH 11/22] libelf: check all pointer accesses Date: Tue, 11 Jun 2013 16:28:11 +0100 Message-ID: <20919.16907.767547.453382@mariner.uk.xensource.com> References: <1370629642-6990-1-git-send-email-ian.jackson@eu.citrix.com> <1370629642-6990-12-git-send-email-ian.jackson@eu.citrix.com> <51B66368.6010602@citrix.com> Mime-Version: 1.0 Content-Type: text/plain; charset="us-ascii" Content-Transfer-Encoding: 7bit Return-path: In-Reply-To: <51B66368.6010602@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: Andrew Cooper Cc: "xen-devel@lists.xensource.com" , "mattjd@gmail.com" , "security@xen.org" List-Id: xen-devel@lists.xenproject.org Andrew Cooper writes ("Re: [PATCH 11/22] libelf: check all pointer accesses"): > On 07/06/13 19:27, Ian Jackson wrote: > > diff --git a/tools/libxc/xc_dom_elfloader.c b/tools/libxc/xc_dom_elfloader.c > > index b8089bc..c038d1c 100644 > > --- a/tools/libxc/xc_dom_elfloader.c > > +++ b/tools/libxc/xc_dom_elfloader.c > > @@ -128,20 +128,30 @@ static int xc_dom_load_elf_symtab(struct xc_dom_image *dom, > > > > if ( load ) > > { > > - size_t allow_size; /* will be used in a forthcoming XSA-55 patch */ > > + char *hdr_ptr; > > + size_t allow_size; > > + > > if ( !dom->bsd_symtab_start ) > > return 0; > > size = dom->kernel_seg.vend - dom->bsd_symtab_start; > > - hdr = xc_dom_vaddr_to_ptr(dom, dom->bsd_symtab_start, &allow_size); > > - *(int *)hdr = size - sizeof(int); > > + hdr_ptr = xc_dom_vaddr_to_ptr(dom, dom->bsd_symtab_start, &allow_size); > > + elf->caller_xdest_base = hdr_ptr; > > + elf->caller_xdest_size = allow_size; > > + hdr = ELF_REALPTR2PTRVAL(hdr_ptr); > > + elf_store_val(elf, int, hdr, size - sizeof(int)); > > } > > Is it not sensible to move the later "check failure of > xc_dom_*_to_ptr()" to here? It would certainly fall into the category > of "check all pointer accesses". I don't have a strong an opinion about where that hunk should go. But arguments against moving it to this patch: * This patch is already complicated enough; * We aren't actually changing the semantics here very much although there's a lot of code churn, so it's obviously not making matters worse; * There are a lot of other call sites which use xc_dom_*_to_ptr unchecked, at this point in the series. It might be possible to move the whole xc_dom_*_to_ptr NULL checking to _before_ all of these invasive patches but that would be a whole lot of work and I'd probably mess up some of the massive amounts of conflict resolution which would be needed. > > diff --git a/xen/common/libelf/libelf-dominfo.c b/xen/common/libelf/libelf-dominfo.c > > index ba0dc83..b9a4e25 100644 > > --- a/xen/common/libelf/libelf-dominfo.c > > +++ b/xen/common/libelf/libelf-dominfo.c > > @@ -254,7 +254,7 @@ int elf_xen_parse_guest_info(struct elf_binary *elf, > > int len; > > > > h = parms->guest_info; > > -#define STAR(h) (*(h)) > > +#define STAR(h) (elf_access_unsigned(elf, (h), 0, 1)) > > Similar query regarding elf_access_unsigned(elf, (h), 0, sizeof(*h)) No, h doesn't have a suitable type. After this patch h is an elf_ptrval, ie a uintptr_t. Hence the use of elf_access_unsigned rather than (say) elf_uval. > > ELF_PTRVAL_VOID elf_get_ptr(struct elf_binary *elf, unsigned long addr) > > { > > - return elf->dest + addr - elf->pstart; > > + return ELF_REALPTR2PTRVAL(elf->dest_base) + addr - elf->pstart; > > Both callsites of this function have addr as a guest supplied > parameter. It needs to be checked for overflowing. No, it doesn't. The returned value is an elf_ptrval. That is, it's a maybe-wrong pseudopointer. Any callers which want to dereference it will have to convert it back somehow, typically using elf_access_unsigned or the like. The arithmetic is all done using unsigned integers so overflows don't matter. They just produce wrong answers which are tolerated. > > diff --git a/xen/include/xen/libelf.h b/xen/include/xen/libelf.h > > index 0c2c11e..783f125 100644 > > --- a/xen/include/xen/libelf.h > > +++ b/xen/include/xen/libelf.h ... > > /* ------------------------------------------------------------------------ */ > > > > - > > Spurious whitespace change. Fixed. Thanks, Ian.