Re: [Xen-devel] [PATCH 18/23] libxc: Add range checking to xc_dom_binloader

[Ian Jackson <Ian.Jackson@xxxxxxxxxxxxx>]

George Dunlap writes ("Re: [Xen-devel] [PATCH 18/23] libxc: Add range checking 
to xc_dom_binloader"):
> On Thu, Jun 13, 2013 at 7:14 PM, Ian Jackson <ian.jackson@xxxxxxxxxxxxx> 
> wrote:
> >  * When clamping the end of the region to search, that we do not
> >    calculate pointers beyond the end of the image.  The C
> >    specification does not permit this and compilers are becoming ever
> >    more determined to miscompile code when they can "prove" various
> >    falsehoods based on assertions from the C spec.
...
> > +    if ( dom->kernel_size < sizeof(*table) )
> > +        return NULL;
> >      probe_ptr = dom->kernel_blob;
> >      probe_end = dom->kernel_blob + dom->kernel_size - sizeof(*table);
> > -    if ( (void*)probe_end > (dom->kernel_blob + 8192) )
> > +    if ( dom->kernel_size >= 8192 &&
> > +         (void*)probe_end > (dom->kernel_blob + 8192) )
> >          probe_end = dom->kernel_blob + 8192;
> 
> Wait, what's going on here?  Isn't the point of this check originally
> that "probe_end" might be pointing off into nowhere, and you're going
> to "clip" it into pointing somewhere reasonable?

No.  The (undocumented) format being parsed here is that the info
table should start no later than 8k into the file.  probe_end is the
first address to no longer look for the table at.

Firstly we set probe_end to the end of the image minus the table
length - which would imply searching the whole image.  The (original
and unchanged) purpose of the if statement and assignment is to limit
this to 8192 bytes from the start of the image.

However there is a bug: if the image is less than 8k long, this
involves computing a wild pointer (which is forbidden).  So in my
patch I add an additional test.

> It doesn't look like you've actually changed any pointer arithmetic --
> if either probe_end or dom->kernel_blob + 8192 are wild, then they'll
> still be wild after this check, won't they?

The initial value of probe_end is guaranteed not to be wild.

Before my change  dom->kernel_blob + 8192  might be computed even if
it is wild; after my change it is only computed if it is guaranteed
not to be.

Ian.

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