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

["Jan Beulich" <JBeulich@xxxxxxxx>]

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

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

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

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

Jan



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