Re: [Xen-devel] [PATCH SpectreV1+L1TF v6 8/9] common/grant_table: block speculative out-of-bound accesses

["Jan Beulich" <JBeulich@xxxxxxxx>]

>>> On 18.02.19 at 14:49, <nmanthey@xxxxxxxxx> wrote:
> On 2/15/19 11:34, Jan Beulich wrote:
>>>>> On 15.02.19 at 10:55, <nmanthey@xxxxxxxxx> wrote:
>>> On 2/13/19 12:50, Jan Beulich wrote:
>>>>>>> On 08.02.19 at 14:44, <nmanthey@xxxxxxxxx> wrote:
>>>>> Guests can issue grant table operations and provide guest controlled
>>>>> data to them. This data is also used for memory loads. To avoid
>>>>> speculative out-of-bound accesses, we use the array_index_nospec macro
>>>>> where applicable. However, there are also memory accesses that cannot
>>>>> be protected by a single array protection, or multiple accesses in a
>>>>> row. To protect these, a nospec barrier is placed between the actual
>>>>> range check and the access via the block_speculation macro.
>>>>>
>>>>> As different versions of grant tables use structures of different size,
>>>>> and the status is encoded in an array for version 2, speculative
>>>>> execution might touch zero-initialized structures of version 2 while
>>>>> the table is actually using version 1.
>>>> Why zero-initialized? Did I still not succeed demonstrating to you
>>>> that speculation along a v2 path can actually overrun v1 arrays,
>>>> not just access parts with may still be zero-initialized?
>>> I believe a speculative v2 access can touch data that has been written
>>> by valid v1 accesses before, zero initialized data, or touch the NULL
>>> page. Given the macros for the access I do not believe that a v2 access
>>> can touch a page that is located behind a page holding valid v1 data.
>> I've given examples before of how I see this to be possible. Would
>> you mind going back to one of the instances, and explaining to me
>> how you do _not_ see any room for an overrun there? Having
>> given examples, I simply don't know how else I can explain this to
>> you without knowing at what specific part of the explanation we
>> diverge. (And no, I'm not excluding that I'm making up an issue
>> where there is none.)
> What we want to real out is that the actually use version1, while
> speculation might use version2, right? I hope you refer to this example
> of your earlier email.
> 
> On 1/29/19 16:11, Jan Beulich wrote:
>> Let's look at an example: gref 256 points into the middle of
>> the first page when using v1 calculations, but at the start
>> of the second page when using v2 calculations. Hence, if the
>> maximum number of grant frames was 1, we'd overrun the
>> array, consisting of just a single element (256 is valid as a
>> v1 gref in that case, but just out of bounds as a v2 one).
> 
> From how I read your example and my explanation, the key difference is
> in the size of the shared_raw array. In case gref 256 is a valid v1
> handle, then the shared_raw array has space for at least 256 entries, as
> shared_raw was allocated for the number of requested entries. The access
> to shared_raw is controlled with the macro shared_entry_v2:
>  222 #define SHGNT_PER_PAGE_V2 (PAGE_SIZE / sizeof(grant_entry_v2_t))
>  223 #define shared_entry_v2(t, e) \
>  224     ((t)->shared_v2[(e)/SHGNT_PER_PAGE_V2][(e)%SHGNT_PER_PAGE_V2])
> Since the direct access to the shared_v2 array depends on the
> SHGNT_PER_PAGE_V2 value, this has to be less than the size of that
> array. Hence, shared_raw will not be overrun (neither for version 1 nor
> version 2). However, this division might result in accessing an element
> of shared_raw that has not been initialized by version1 before. However,
> right after allocation, shared_raw is zero initialized. Hence, this
> might result in an access of the NULL page.

The question is: How much of shared_raw[] will be zero-initialized?
The example I've given uses relatively small grant reference values,
so for the purpose here let's assume gt->max_grant_frames is 1.
In this case shared_raw[] is exactly one entry in size. Hence the
speculative access you describe will not necessarily access the NULL
page.

Obviously the same issue exists with higher limits and higher grant
reference numbers.

>>>>> @@ -1321,7 +1327,8 @@ unmap_common(
>>>>>          goto unlock_out;
>>>>>      }
>>>>>  
>>>>> -    act = active_entry_acquire(rgt, op->ref);
>>>>> +    act = active_entry_acquire(rgt, array_index_nospec(op->ref,
>>>>> +                                                       
>>> nr_grant_entries(rgt)));
>>>> ... you add a use e.g. here to _guard_ against speculation.
>>> The adjustment you propose is to exchange the switch statement in
>>> nr_grant_entries with a if( evaluate_nospec( gt->gt_version == 1 ), so
>>> that the returned values are not speculated?
>> At this point I'm not proposing a particular solution. I'm just
>> putting on the table an issue left un-addressed. I certainly
>> wouldn't welcome converting the switch() to an if(), even if
>> right now there's no v3 on the horizon. (It's actually quite
>> the inverse: If someone came and submitted a patch to change
>> the various if()-s on gt_version to switch()-es, I'd welcome this.)
> I am happy to add block_speculation() macros into each case of the
> switch statement.

Ugly, but perhaps the only possible solution at this point.

>>> Do you want me to
>>> cache the value in functions that call this method regularly to avoid
>>> the penalty of the introduced lfence for each call?
>> That would go back to the question of what good it does to
>> latch value into a local variable when you don't know whether
>> the compiler will put that variable in a register or in memory.
>> IOW I'm afraid that to be on the safe side there's no way
>> around the repeated LFENCEs.
> The difference here would be that in case the value is stored into a
> local variable (independently of memory or register) and an lfence was
> executed, this value can be trusted and does not have to be checked
> again, as it's no longer guest controlled.

Ah, yes, you're right (it just wasn't clear to me that you implied
adding a fence together with the caching of the value). So perhaps
that's then also the way to go for the hunks under discussion in
patch 3?

Jan


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