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

[George Dunlap <George.Dunlap@xxxxxxxxxxxxx>]

On Fri, Jun 14, 2013 at 4:09 PM, George Dunlap
<george.dunlap@xxxxxxxxxxxxx> wrote:
> 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>

Although, having said that, wouldn't it be clearer to do something like:

if ( dom->kernel-size - sizeof(*table) > 8192)
  ptr_end = dom->kernel_blob + 8192
else
  ptr_end = dom->kernel_size - sizeof(*table);

Or something along those lines?

 -George

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