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

[George Dunlap <george.dunlap@eu.citrix.com>]

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@eu.citrix.com> 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@eu.citrix.com>