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

["Jan Beulich" <JBeulich@suse.com>]

>>> 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).

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.

Jan



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