Re: [Xen-devel] [PATCH 19/22] libxc: check failure of xc_dom_*_to_ptr, xc_map_foreign_range

[Ian Jackson <Ian.Jackson@xxxxxxxxxxxxx>]

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