[Date Prev][Date Next][Thread Prev][Thread Next][Date Index][Thread Index]

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

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

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?


Xen-devel mailing list



Lists.xenproject.org is hosted with RackSpace, monitoring our
servers 24x7x365 and backed by RackSpace's Fanatical Support®.