Re: [Xen-devel] [PATCH SpectreV1+L1TF v4 08/11] xen/evtchn: block speculative out-of-bound accesses

["Jan Beulich" <JBeulich@xxxxxxxx>]

>>> On 23.01.19 at 12:57, <nmanthey@xxxxxxxxx> wrote:
> --- a/xen/common/event_channel.c
> +++ b/xen/common/event_channel.c
> @@ -368,8 +368,14 @@ int evtchn_bind_virq(evtchn_bind_virq_t *bind, 
> evtchn_port_t port)
>      if ( virq_is_global(virq) && (vcpu != 0) )
>          return -EINVAL;
>  
> +   /*
> +    * Make sure the guest controlled value virq is bounded even during
> +    * speculative execution.
> +    */
> +    virq = array_index_nospec(virq, ARRAY_SIZE(v->virq_to_evtchn));

I think this wants to move ahead of the if() in context, to be independent
of the particular implementation of virq_is_global() (the current shape of
which is mostly fine, perhaps with the exception of the risk of the compiler
translating the switch() there by way of a jump table). This also moves it
closer to the if() the construct is a companion to.

> @@ -816,6 +822,12 @@ int set_global_virq_handler(struct domain *d, uint32_t 
> virq)
>      if (!virq_is_global(virq))
>          return -EINVAL;
>  
> +   /*
> +    * Make sure the guest controlled value virq is bounded even during
> +    * speculative execution.
> +    */
> +    virq = array_index_nospec(virq, ARRAY_SIZE(global_virq_handlers));

Same here then.

> @@ -931,7 +943,8 @@ long evtchn_bind_vcpu(unsigned int port, unsigned int 
> vcpu_id)
>      struct evtchn *chn;
>      long           rc = 0;
>  
> -    if ( (vcpu_id >= d->max_vcpus) || (d->vcpu[vcpu_id] == NULL) )
> +    if ( (vcpu_id >= d->max_vcpus) ||
> +         (d->vcpu[array_index_nospec(vcpu_id, d->max_vcpus)] == NULL) )
>          return -ENOENT;
>  
>      spin_lock(&d->event_lock);
> @@ -969,8 +982,10 @@ long evtchn_bind_vcpu(unsigned int port, unsigned int 
> vcpu_id)
>          unlink_pirq_port(chn, d->vcpu[chn->notify_vcpu_id]);
>          chn->notify_vcpu_id = vcpu_id;
>          pirq_set_affinity(d, chn->u.pirq.irq,
> -                          cpumask_of(d->vcpu[vcpu_id]->processor));
> -        link_pirq_port(port, chn, d->vcpu[vcpu_id]);
> +                          cpumask_of(d->vcpu[array_index_nospec(vcpu_id,
> +                                                                
> d->max_vcpus)]->processor));
> +        link_pirq_port(port, chn, d->vcpu[array_index_nospec(vcpu_id,
> +                                                             d->max_vcpus)]);

Using Andrew's new domain_vcpu() will improve readability, especially
after your change, quite a bit here. But of course code elsewhere will
benefit as well.

Jan



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