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:12 PM, George Dunlap
<George.Dunlap@xxxxxxxxxxxxx> wrote:
> 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);

I mean

ptr_end = dom->kernel_blob + dom->kernel_size - sizeof(*table);

here of course...

 -G

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