[Date Prev][Date Next][Thread Prev][Thread Next][Date Index][Thread Index] Re: [Xen-devel] [PATCH 19/22] libxc: check failure of xc_dom_*_to_ptr, xc_map_foreign_range
Andrew Cooper writes ("Re: [PATCH 19/22] libxc: check failure of xc_dom_*_to_ptr, xc_map_foreign_range"): > On 07/06/13 19:27, 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. ... > > + } > > Consistency of __FUNCTION__ vs __func__ ? > > It looks to be inconsistent across the codebase, but __FUNCTION__ is > nonstandard whereas __func__ is specified by C99. __FUNCTION__ is a GCC extension. Quite an ancient one. I copied the style from the nearby code. I suggest we fix this later if we care. > > diff --git a/tools/libxc/xc_dom_elfloader.c b/tools/libxc/xc_dom_elfloader.c > > index 1d2727e..ac9bb3b 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/%s: xc_dom_vaddr_to_ptr(,dom->bsd_symtab_start" > > + " => NULL", __FUNCTION__, load ? "load" : "parse"); > > + return -1; > > + } > > Here, you are in an "if ( load )" block, so the conditional operator on > load is unnecessary. Oh yes. > There is however an xc_dom_malloc() in the associated else block lacking > any printing, which would line up with the "parse" half. So there is. That ought to be fixed later, as it's a pre-existing non-security bug. (For greater consistency I could leave out the new DOMPRINTF but I think that's worse.) > Also, consistency of error messages. Previously you have had "(dom, > ...)" instead of "(," Will fix. > > @@ -383,6 +389,13 @@ static elf_errorstatus xc_dom_load_elf_kernel(struct > > xc_dom_image *dom) > > > > elf->dest_base = xc_dom_seg_to_ptr_pages(dom, &dom->kernel_seg, > > &pages); > > elf->dest_size = pages * XC_DOM_PAGE_SIZE(dom); > > + if ( elf->dest_base == NULL ) > > + { > > + DOMPRINTF("%s: xc_dom_vaddr_to_ptr(,dom->kernel_seg)" > > + " => NULL", __FUNCTION__); > > + return -1; > > + } > > + > > You set elf->dest_size before checking for a NULL pointer on elf->dest_base. > > As 'pages' will be 0 in the case of a failed xc_dom_seg_to_ptr_pages, it > should be safe, but should probably be fixed. Yes, it should. Thanks, Ian. _______________________________________________ Xen-devel mailing list Xen-devel@xxxxxxxxxxxxx http://lists.xen.org/xen-devel
|
Lists.xenproject.org is hosted with RackSpace, monitoring our |