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

[Norbert Manthey <nmanthey@amazon.de>]

On 1/24/19 17:56, Jan Beulich wrote:
>>>> On 23.01.19 at 12:57, <nmanthey@amazon.de> 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?
>
>> @@ -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.

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.

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