Re: [Xen-devel] [PATCH SpectreV1+L1TF v4 09/11] x86/vioapic: block speculative out-of-bound accesses

[Norbert Manthey <nmanthey@xxxxxxxxx>]

On 1/25/19 17:34, Jan Beulich wrote:
>>>> On 23.01.19 at 12:57, <nmanthey@xxxxxxxxx> wrote:
>> @@ -66,6 +67,9 @@ static struct hvm_vioapic *gsi_vioapic(const struct domain 
>> *d,
>>  {
>>      unsigned int i;
>>  
>> +    /* Make sure the compiler does not optimize the initialization */
>> +    OPTIMIZER_HIDE_VAR(pin);
> Since there's no initialization here, I think it would help to add "done
> in the callers". Perhaps also "optimize away" or "delete"?
>
> And then I think you mean *pin.
True, I will adapt both the comment and the OPTIMIZER_HIDE_VAR call.
>
>> @@ -212,7 +217,12 @@ 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;
>> +
>> +    /* Make sure no out-of-bound value for idx can be used */
>> +    idx = array_index_nospec(idx, vioapic->nr_pins);
>> +
>> +    gsi = vioapic->base_gsi + idx;
> I dislike the disconnect from the respective bounds check: There's
> only one caller, so the construct could be moved there, or
> otherwise I'd like to see an ASSERT() added documenting that the
> bounds check is expected to have happened in the caller.
I agree that the idx value is used as an array index in this function
only once. However, the gsi value also uses the value of idx, and as
that is passed to other functions, I want to bound the gsi variable as
well. Therefore, I chose to have a separate assignment for the idx variable.
>
>> @@ -378,7 +388,8 @@ static inline int pit_channel0_enabled(void)
>>  
>>  static void vioapic_deliver(struct hvm_vioapic *vioapic, unsigned int pin)
>>  {
>> -    uint16_t dest = vioapic->redirtbl[pin].fields.dest_id;
>> +    uint16_t dest = vioapic->redirtbl
>> +               [pin = array_index_nospec(pin, 
>> vioapic->nr_pins)].fields.dest_id;
>>      uint8_t dest_mode = vioapic->redirtbl[pin].fields.dest_mode;
>>      uint8_t delivery_mode = vioapic->redirtbl[pin].fields.delivery_mode;
>>      uint8_t vector = vioapic->redirtbl[pin].fields.vector;
> I'm sorry, but despite prior discussions I'm still not happy about
> this change - all of the callers pass known good values:
> - vioapic_write_redirent() gets adjusted above,
> - vioapic_irq_positive_edge() gets the value passed into here
>   from gsi_vioapic(), which you also take care of,
> - vioapic_update_EOI() loops over all pins, so only passes in-
>   range values.
> Therefore I still don't see what protection this change adds.
> As per above, if it was to stay, some sort of connection to the
> range check(s) it guards would otherwise be nice to establish,
> but I realize that adding an ASSERT() here would go against
> a certain aspect of review comments I gave on earlier versions.
I will drop this change. As you called out, all callers are bound
checked already. Hence, I will not add an assert.
>
>> @@ -463,7 +474,7 @@ static void vioapic_deliver(struct hvm_vioapic *vioapic, 
>> unsigned int pin)
>>  
>>  void vioapic_irq_positive_edge(struct domain *d, unsigned int irq)
>>  {
>> -    unsigned int pin;
>> +    unsigned int pin = 0; /* See gsi_vioapic */
>>      struct hvm_vioapic *vioapic = gsi_vioapic(d, irq, &pin);
>>      union vioapic_redir_entry *ent;
>>  
>> @@ -560,7 +571,7 @@ int vioapic_get_vector(const struct domain *d, unsigned 
>> int gsi)
>>  
>>  int vioapic_get_trigger_mode(const struct domain *d, unsigned int gsi)
>>  {
>> -    unsigned int pin;
>> +    unsigned int pin = 0; /* See gsi_vioapic */
>>      const struct hvm_vioapic *vioapic = gsi_vioapic(d, gsi, &pin);
>>  
>>      if ( !vioapic )
> Since there are more callers of gsi_vioapic(), justification should be
> added to the description why only some need adjustment (or
> otherwise, just to be on the safe side as well as for consistency
> all of them should be updated, in which case it would still be nice
> to call out the ones which really [don't] need updating).

I will extend the explanation in the commit message.

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