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

[Norbert Manthey <nmanthey@amazon.de>]

On 1/29/19 16:11, Jan Beulich wrote:
>>>> On 29.01.19 at 14:47, <nmanthey@amazon.de> wrote:
>> On 1/29/19 10:46, Jan Beulich wrote:
>>>>>> Norbert Manthey <nmanthey@amazon.de> 01/29/19 9:35 AM >>>
>>>> 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.
>>> And this is the problem afaics: If a guest has requested its grant table to
>>> be sized as a single page, this page can fit twice as many entries for
>>> v1 than it can fit for v2. Hence the v1 grant reference pointing at the last
>>> entry would point at the last entry in the (not mapped) second page for v2.
>> I might understand the code wrong, but a guest would ask to get at most
>> N grant frames, and this number cannot be increased afterwards, i.e. the
>> field gt->max_grant_frames is written exactly once. Furthermore, the
>> void** shared_raw array is allocated and written exactly once with
>> sufficient pointers for, namely gt->max_grant_frames many in function
>> grant_table_init. Hence, independently of the version being used, at
>> least the shared_raw array cannot be used for out-of-bound accesses
>> during speculation with my above evaluate_nospec.
> I'm afraid I'm still not following: A give number of pages is worth
> twice as many grants in v1 than it is in v2. Therefore a v1 grant
> reference to a grant entry tracked in the second half of the
> first page would cause a speculative access to anywhere in the
> second page when wrongly interpreted as a v2 ref.
Agreed. So you want me to add another lfence to make sure the wrong
interpretation does not lead to other out-of-bound accesses down the
speculative window? In my opinion, the v1 vs v2 code does not result in
actual out-of-bound accesses, except for the NULL page case below. To
make the PV case happy, I will add the evaluate_nospec macro for the v1
vs v2 conditionals in functions with guest controlled ref indexes.
>
>> That being said, let's assume we have a v1 grant table, and speculation
>> uses the v2 accesses. In that case, an existing and zero-initialized
>> entry of shared_raw might be used in the first part of the
>> shared_entry_v2 macro, and even if that pointer would be non-NULL, the
>> page it would point to would have been cleared when growing the grant
>> table in function gnttab_grow_table.
> Not if the v1 ref is no smaller than half the maximum number of
> v1 refs. In that case, if taken as a v2 ref, ->shared_raw[]
> would need to be twice as big to cope with the larger index
> (resulting from the smaller divisor in shared_entry_v2()
> compared to shared_entry_v1()) in order to not be overrun.
>
> 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).
If 256 is a valid gref, then the shared_raw array holds sufficient
zero-initialized elements for such an access, even without the division
operator that is used in the shared_entry_v*() macros. Hence, no
out-of-bound access will happen here.
>
> Furthermore, even if ->shared_raw[] itself could not be overrun,
> an entry of it being NULL could be a problem with PV guests, who
> can install a translation for the first page of the address space,
> and thus perhaps partly control subsequent speculative execution.

I understand the concern. I add the evaluate_nospec as mentioned above.

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