Re: [Xen-devel] [PATCH L1TF MDS GT v4 1/2] common/grant_table: harden bound accesses

[Norbert Manthey <nmanthey@xxxxxxxxx>]

On 7/30/19 15:38, Jan Beulich wrote:
> On 30.07.2019 15:15, Norbert Manthey wrote:
>> Guests can issue grant table operations and provide guest controlled
>> data to them. This data is used as index for memory loads after bound
>> checks have been done. To avoid speculative out-of-bound accesses, we
>> use the array_index_nospec macro where applicable, or the macro
>> block_speculation. Note, the block_speculation macro is used on all
>> path in shared_entry_header and nr_grant_entries. This way, after a
>> call to such a function, all bound checks that happened before become
>> architectural visible, so that no additional protection is required
>> for corresponding array accesses. As the way we introduce an lfence
>> instruction might allow the compiler to reload certain values from
>> memory multiple times, we try to avoid speculatively continuing
>> execution with stale register data by moving relevant data into
>> function local variables.
>>
>> Speculative execution is not blocked in case one of the following
>> properties is true:
>>   - path cannot be triggered by the guest
>>   - path does not return to the guest
>>   - path does not result in an out-of-bound access
>>
>> Only the combination of the above properties allows to actually leak
>> continuous chunks of memory. Therefore, we only add the penalty of
>> protective mechanisms in case a potential speculative out-of-bound
>> access matches all the above properties.
>>
>> This commit addresses only out-of-bound accesses whose index is
>> directly controlled by the guest, and the index is checked before.
>> Potential out-of-bound accesses that are caused by speculatively
>> evaluating the version of the current table are not addressed in this
>> commit. Hence, speculative out-of-bound accesses might still be
>> possible, for example in gnttab_get_status_frame_mfn, when calling
>> gnttab_grow_table, the assertion that the grant table version equals
>> two might not hold under speculation.
>>
>> This is part of the speculative hardening effort.
>>
>> Signed-off-by: Norbert Manthey <nmanthey@xxxxxxxxx>
>> Reviewed-by: Jan Beulich <jbeulich@xxxxxxxx>
>> ---
>>
>> Notes:
>>    v3:  Drop condition to not fix defects in commit message.
>>         Copy in reviewed-by.
> According to this (which aiui means v4) there are no code changes
> compared to v3. At the risk of annoying you, this doesn't fit well
> with me having said "and then perhaps make changes to a few more
> paths" alongside the option of doing this removal in reply to v3.
> After all you've now dropped a condition from what is covered by
> "Only the combination of ...", and hence there's a wider set of
> paths that would need to be fixed. It was for this reason that as
> the other alternative I did suggest to simply weaken the wording
> of the item you've now dropped. IOW I'm afraid my R-b is not
> applicable to v4.

I see, and am sorry for the misunderstanding. I am fine with adding the
4th condition in a weakened form (essentially modifying the commit
message to the form you suggested). I wonder whether the summary when to
fix a potential speculative out-of-bound access should actually be
documented somewhere else than in the commit message of this (more or
less random) commit.

Best,
Norbert

>
> Jan



Amazon Development Center Germany GmbH
Krausenstr. 38
10117 Berlin
Geschaeftsfuehrung: Christian Schlaeger, Ralf Herbrich
Eingetragen am Amtsgericht Charlottenburg unter HRB 149173 B
Sitz: Berlin
Ust-ID: DE 289 237 879


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