Re: [Xen-devel] [PATCH 10/16] libelf: check all pointer accesses

[Ian Campbell <Ian.Campbell@xxxxxxxxxx>]

On Fri, 2013-06-07 at 00:25 +1200, Matthew Daley wrote:
> On Wed, Jun 5, 2013 at 5:59 AM, Ian Jackson <ian.jackson@xxxxxxxxxxxxx> wrote:
> [...]
> > @@ -128,19 +128,31 @@ static int xc_dom_load_elf_symtab(struct xc_dom_image 
> > *dom,
> >
> >      if ( load )
> >      {
> > +        char *hdr_ptr;
> > +        unsigned int page_size = XC_DOM_PAGE_SIZE(dom);
> > +
> >          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);
> > -        *(int *)hdr = size - sizeof(int);
> > +        hdr_ptr  = xc_dom_vaddr_to_ptr(dom, dom->bsd_symtab_start);
> > +        elf->caller_xdest_base = hdr_ptr;
> 
> This result needs to be checked against NULL, otherwise a silly
> dom->bsd_symtab_start will mistakenly allow access to the base of
> address space.

I think this may be looking up the address previously allocated in the !
load else case of this if using bsd_symtab_start as a key. The
allocation in that else case is properly checked. Given that
bsd_symtab_start cannot be changed under our feet if I'm right then  the
only way you could get NULL here would be if our internal bookkeeping
had gotten messed up.

IOW I think abort() would be an appropriate response.

> [...]
> > @@ -309,8 +344,10 @@ static int xc_dom_load_elf_kernel(struct xc_dom_image 
> > *dom)
> >  {
> >      struct elf_binary *elf = dom->private_loader;
> >      int rc;
> > +    xen_pfn_t pages;
> >
> > -    elf->dest = xc_dom_seg_to_ptr(dom, &dom->kernel_seg);
> > +    elf->dest_base = xc_dom_seg_to_ptr_pages(dom, &dom->kernel_seg, 
> > &pages);
> 
> Similarly, but with a silly dom->kernel_seg.

I don't know whether or not this one is "prevalidated" like the vaddr
case.

> (crashsig 843a77d)

Does this indicate that you've observed the xc_dom_vaddr or the
xc_dom_seg one or both?

Obviously if it is the former or both then I am talking out my behind...

Ian.


_______________________________________________
Xen-devel mailing list
Xen-devel@xxxxxxxxxxxxx
http://lists.xen.org/xen-devel