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

[Norbert Manthey <nmanthey@amazon.de>]

On 2/18/19 17:08, Jan Beulich wrote:
>>>> On 18.02.19 at 14:49, <nmanthey@amazon.de> wrote:
>> On 2/15/19 11:34, Jan Beulich wrote:
>>>>>> On 15.02.19 at 10:55, <nmanthey@amazon.de> wrote:
>>>> On 2/13/19 12:50, Jan Beulich wrote:
>>>>>>>> On 08.02.19 at 14:44, <nmanthey@amazon.de> 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.
The solution to this problem is really simple, I mixed up grant frames
and grant entries. I agree that shared_raw can be accessed out-of-bounds
and should be protected. I will adapt the commit message accordingly,
and revise the modifications I added to the code base.
>
>>>>>> @@ -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?

Well, yes. That should effectively bound the a.index values in the other
hunks in patch 3 as well. I will adapt that patch accordingly. Until
now, I stepped back from this solution, as I want to avoid using the
lfence instruction as much as possible.

Best,
Norbert





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