Re: [Xen-devel] [PATCH SpectreV1+L1TF v5 2/9] x86/vioapic: block speculative out-of-bound accesses

[Norbert Manthey <nmanthey@xxxxxxxxx>]

On 1/31/19 17:05, Jan Beulich wrote:
>>>> On 29.01.19 at 15:43, <nmanthey@xxxxxxxxx> wrote:
>> When interacting with io apic, a guest can specify values that are used
>> as index to structures, and whose values are not compared against
>> upper bounds to prevent speculative out-of-bound accesses. This change
>> prevents these speculative accesses.
>>
>> Furthermore, two variables are initialized and the compiler is asked to
>> not optimized these initializations, as the uninitialized, potentially
>> guest controlled, variables might be used in a speculative out-of-bound
>> access. As the two problematic variables are both used in the common
>> function gsi_vioapic, the mitigation is implemented there. Currently,
>> the problematic callers are the functions vioapic_irq_positive_edge and
>> vioapic_get_trigger_mode.
> I would have wished for you to say why the other two are _not_
> a problem. Afaict in both cases the functions only ever get
> internal data passed.
>
> Then again I'm not convinced it's worth taking the risk that a
> problematic caller gets added down the road. How about you add
> initializers everywhere, clarifying in the description that it's "just
> in case" for the two currently safe ones?
I will add the other initialization and update the commit message.
>
>> This commit is part of the SpectreV1+L1TF mitigation patch series.
>>
>> Signed-off-by: Norbert Manthey <nmanthey@xxxxxxxxx>
>>
>> ---
> Btw., could you please get used to the habit of adding a brief
> summary of changes for at least the most recent version here,
> which aids review quite a bit?
I will start to do this with the next version.
>
>> @@ -212,7 +220,15 @@ static void vioapic_write_redirent(
>>      struct hvm_irq *hvm_irq = hvm_domain_irq(d);
>>      union vioapic_redir_entry *pent, ent;
>>      int unmasked = 0;
>> -    unsigned int gsi = vioapic->base_gsi + idx;
>> +    unsigned int gsi;
>> +
>> +    /* Callers of this function should make sure idx is bounded 
>> appropriately*/
> Missing blank at the end of the comment (which, if this was the
> only open point, would be easy enough to adjust while committing).

Will fix.

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@xxxxxxxxxxxxxxxxxxxx
https://lists.xenproject.org/mailman/listinfo/xen-devel