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

["Jan Beulich" <JBeulich@xxxxxxxx>]

>>> On 30.01.19 at 09:06, <nmanthey@xxxxxxxxx> wrote:
> On 1/29/19 16:11, Jan Beulich wrote:
>>>>> On 29.01.19 at 14:47, <nmanthey@xxxxxxxxx> wrote:
>>> On 1/29/19 10:46, Jan Beulich wrote:
>>>>>>> Norbert Manthey <nmanthey@xxxxxxxxx> 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.

Please don't get me wrong - I'm not saying these have to be added.
The context here was that you added some checks but not others.
Going forward it is likely going to be important (or at least helpful)
to know where the boundaries are drawn. This is the more that I
think we all agree that insertion rules (or should I say guide lines)
are fuzzy enough already as long as we don't choose to guard _all_
conditionals.

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

There's no such thing as "256 is a valid gref" without saying for
what version. Since the shared table setup is driven by a number
of page frames, the number of valid grefs depends on the
version. In the single page example, 256 is a valid gref for v1,
but an out of bounds one for v2. Speculation along a v2 path
would access ->shared_raw[1] which was not set up (when
actually using v1), nor even allocated space for to store a NULL
in if ->max_grant_frames was 1.

Jan


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