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

["Jan Beulich" <JBeulich@xxxxxxxx>]

>>> On 24.01.19 at 20:50, <nmanthey@xxxxxxxxx> wrote:
> On 1/24/19 17:56, Jan Beulich wrote:
>>>>> 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.
> I understand the concern. However, because the value of virq would be
> changed before the virq_is_global check, couldn't that result in
> returning a wrong error code? The potential out-of-bound value is
> brought back into the valid range, so that the above check might fire
> incorrectly?

No - and incorrect (out of bounds value) making it into virq_is_global()
is possible during mis-speculation only anyway. Out of range values,
for the purpose of architecturally visible state, get rejected by the
first if(). In range values won't be altered by array_index_nospec().

>>> @@ -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.
> 
> You mean I should use the domain_vcpu function in both hunks, because
> due to the first one, the latter can never return NULL? I will rebase
> the series on top of this fresh change, and use the domain_vcpu function
> for the locations where I bound a vcpu_id.

Thanks - that why Andrew had dusted off this old change of his.

Jan



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