[Date Prev][Date Next][Thread Prev][Thread Next][Date Index][Thread Index]
Re: [Xen-devel] [PATCH 18/23] libxc: Add range checking to xc_dom_binloader
On 14/06/13 16:03, Ian Jackson wrote:
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.
Oh, I see -- right; so if "dom->kernel_size < 8192", then
"dom->kernel_blob+8192" might be wild; but as long as "dom->kernel_size
> 8192", then "dom->kernel_blob + 8192" will be fine.
And if dom->kernel_size < 8192, then probe_end will already be <
dom->kernel_blob+8192, so we don't need to clip things.
Might be nice to put a comment here, so people coming later can make
sense out of this. Even if people read the changeset description, they
may, like me, not be able to suss out which calculation is in danger of
being wild.
Other than that:
Reviewed-by: George Dunlap <george.dunlap@xxxxxxxxxxxxx>
_______________________________________________
Xen-devel mailing list
Xen-devel@xxxxxxxxxxxxx
http://lists.xen.org/xen-devel
|
