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

[Matthew Daley <mattjd@xxxxxxxxx>]

On Fri, Jun 7, 2013 at 3:30 AM, Ian Campbell <Ian.Campbell@xxxxxxxxxx> wrote:
> 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.

That xc_dom_malloc function doesn't allocate pages for
xc_dom_pfn_to_ptr to use, it's just a GC'd wrapper around malloc.

> 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.

In the case I observed, bsd_symtab_start was just calculated with a
silly value to begin with.

>
> 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.

I could have sworn I saw a crash that had dest_base set to NULL
(presumably from this assignment), but I can't find one any more...

>
>> (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...
>

Just the former (apparently).

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