Re: [PATCH 11/22] libelf: check all pointer accesses

[Ian Jackson <Ian.Jackson@eu.citrix.com>]

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.