From mboxrd@z Thu Jan  1 00:00:00 1970
From: Ian Jackson <Ian.Jackson@eu.citrix.com>
Subject: Re: [PATCH 19/22] libxc: check failure of
	xc_dom_*_to_ptr, xc_map_foreign_range
Date: Tue, 11 Jun 2013 15:44:42 +0100
Message-ID: <20919.14298.751552.111738@mariner.uk.xensource.com>
References: <1370629642-6990-1-git-send-email-ian.jackson@eu.citrix.com>
	<1370629642-6990-20-git-send-email-ian.jackson@eu.citrix.com>
	<51B640A3.9010509@citrix.com>
Mime-Version: 1.0
Content-Type: text/plain; charset="us-ascii"
Content-Transfer-Encoding: 7bit
Return-path: <xen-devel-bounces@lists.xen.org>
In-Reply-To: <51B640A3.9010509@citrix.com>
List-Unsubscribe: <http://lists.xen.org/cgi-bin/mailman/options/xen-devel>,
	<mailto:xen-devel-request@lists.xen.org?subject=unsubscribe>
List-Post: <mailto:xen-devel@lists.xen.org>
List-Help: <mailto:xen-devel-request@lists.xen.org?subject=help>
List-Subscribe: <http://lists.xen.org/cgi-bin/mailman/listinfo/xen-devel>,
	<mailto:xen-devel-request@lists.xen.org?subject=subscribe>
Sender: xen-devel-bounces@lists.xen.org
Errors-To: xen-devel-bounces@lists.xen.org
To: Andrew Cooper <andrew.cooper3@citrix.com>
Cc: "xen-devel@lists.xensource.com" <xen-devel@lists.xensource.com>, "mattjd@gmail.com" <mattjd@gmail.com>, "security@xen.org" <security@xen.org>
List-Id: xen-devel@lists.xenproject.org

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.