Re: [PATCH SpectreV1+L1TF v4 05/11] common/grant_table: block speculative out-of-bound accesses

[Norbert Manthey <nmanthey@amazon.de>]

On 1/28/19 16:09, Jan Beulich wrote:
>>>> On 28.01.19 at 15:45, <nmanthey@amazon.de> wrote:
>> On 1/23/19 14:37, Jan Beulich wrote:
>>>>>> On 23.01.19 at 12:51, <nmanthey@amazon.de> wrote:
>>>> @@ -2223,7 +2231,8 @@ gnttab_transfer(
>>>>          okay = gnttab_prepare_for_transfer(e, d, gop.ref);
>>>>          spin_lock(&e->page_alloc_lock);
>>>>  
>>>> -        if ( unlikely(!okay) || unlikely(e->is_dying) )
>>>> +        /* Make sure this check is not bypassed speculatively */
>>>> +        if ( evaluate_nospec(unlikely(!okay) || unlikely(e->is_dying)) )
>>>>          {
>>>>              bool_t drop_dom_ref = !domain_adjust_tot_pages(e, -1);
>>> What is it that makes this particular if() different from other
>>> surrounding ones? In particular the version dependent code (a few
>>> lines down from here as well as elsewhere) look to be easily
>>> divertable onto the wrong branch, then causing out of bounds
>>> speculative accesses due to the different (version dependent)
>>> shared entry sizes.
>> This check evaluates the variable okay, which indicates whether the
>> value of gop.ref is bounded correctly.
> How does gop.ref come into play here? The if() above does not use
> or update it.
>
>> The next conditional that uses
>> code based on a version should be fine, even when being entered
>> speculatively with the wrong version setup, as the value of gop.ref is
>> correct (i.e. architecturally visible after this lfence) already. As the
>> version dependent macros are used, i.e. shared_entry_v1 and
>> shared_entry_v2, I do not see a risk why speculative out-of-bound access
>> should happen here.
> As said - v2 entries are larger than v1 ones. Therefore, if the
> processor wrongly speculates along the v2 path, it may use
> indexes valid for v1, but beyond the size when scaled by v2
> element size (whereas ->shared_raw[], aliased with
> ->shared_v1[] and ->shared_v2[], was actually set up with v1
> element size).
I am aware that both version use the same base array, and access it via
different macros, which essentially partition the array based on the
size of the respective struct. The underlying raw array has the same
size for both version. In case the CPU decides to enter the wrong
branch, but uses a valid gop.ref value, no out-of-bound accesses will
happen, because in each branch, the accesses via shared_entry_v1 or
shared_entry_v2 make sure the correct math is used to divide the raw
array into chunks of the size of the correct structure. I agree that
speculative execution might access a v1 raw array with v2 offsets, but
that does not result in an out-of-bound access. The data that is used
afterwards might be garbage, here sha->frame. Whether accesses based on
this should be protected could be another discussion, but it at least
looks complex to turn that into an exploitable pattern.
>
> And please don't forget - this subsequent conditional was just an
> easy example. What I'm really after is why you modify the if()
> above, without there being any array index involved.

The check that I protected uses the value of the variable okay, which -
at least after the introduced protecting lfence instruction - holds the
return value of the function gnttab_prepare_for_transfer. This function,
among others, checks whether gop.ref is bounded. By protecting the
evaluation of okay, I make sure to continue only in case gop.ref is
bounded. Consequently, further (speculative) execution is aware of a
valid value of gop.ref.

Best,
Norbert

>
> Jan
>
>




Amazon Development Center Germany GmbH
Krausenstr. 38
10117 Berlin
Geschaeftsfuehrer: Christian Schlaeger, Ralf Herbrich
Ust-ID: DE 289 237 879
Eingetragen am Amtsgericht Charlottenburg HRB 149173 B

_______________________________________________
Xen-devel mailing list
Xen-devel@lists.xenproject.org
https://lists.xenproject.org/mailman/listinfo/xen-devel