Re: [PATCH SpectreV1+L1TF v5 1/9] xen/evtchn: block speculative out-of-bound accesses

[Norbert Manthey <nmanthey@amazon.de>]

On 1/31/19 16:05, Jan Beulich wrote:
>>>> On 29.01.19 at 15:43, <nmanthey@amazon.de> wrote:
>> --- a/xen/common/event_channel.c
>> +++ b/xen/common/event_channel.c
>> @@ -365,11 +365,16 @@ int evtchn_bind_virq(evtchn_bind_virq_t *bind, evtchn_port_t port)
>>      if ( (virq < 0) || (virq >= ARRAY_SIZE(v->virq_to_evtchn)) )
>>          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));
>> +
>>      if ( virq_is_global(virq) && (vcpu != 0) )
>>          return -EINVAL;
>>  
>> -    if ( (vcpu < 0) || (vcpu >= d->max_vcpus) ||
>> -         ((v = d->vcpu[vcpu]) == NULL) )
>> +    if ( (vcpu < 0) || ((v = domain_vcpu(d, vcpu)) == NULL) )
>>          return -ENOENT;
> Is there a reason for the less-than-zero check to survive?
Yes, domain_vcpu uses unsigned integers, and I want to return the proper
error code, in case somebody comes with a vcpu number that would
overflow into the valid range.
>
>> @@ -418,8 +423,7 @@ static long evtchn_bind_ipi(evtchn_bind_ipi_t *bind)
>>      int            port, vcpu = bind->vcpu;
>>      long           rc = 0;
>>  
>> -    if ( (vcpu < 0) || (vcpu >= d->max_vcpus) ||
>> -         (d->vcpu[vcpu] == NULL) )
>> +    if ( (vcpu < 0) || domain_vcpu(d, vcpu) == NULL )
>>          return -ENOENT;
> I'm not sure about this one: We're not after the struct vcpu pointer
> here. Right now subsequent code looks fine, but what if the actual
> "vcpu" local variable was used again in a risky way further down? I
> think here and elsewhere it would be best to eliminate that local
> variable, and use v->vcpu_id only for subsequent consumers (or
> alternatively latch the local variable's value only _after_ the call to
> domain_vcpu(), which might be better especially in cases like).

I agree with getting rid of using the local variable. As discussed
elsewhere, updating such a variable might not fix the problem. However,
in this commit I want to avoid speculative out-of-bound accesses using a
guest controlled variable (vcpu). Hence, I add protection to the
locations where it is used as index. As the domain_vcpu function comes
with protection, I prefer this function over explicitly using
array_index_nospec, if possible.

>
>> @@ -969,8 +980,8 @@ 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(domain_vcpu(d, vcpu_id)->processor));
>> +        link_pirq_port(port, chn, domain_vcpu(d, vcpu_id));
> ... this one, where you then wouldn't need to alter code other than
> that actually checking the vCPU ID.
Instead, I will introduce a struct vcpu variable, assign it in the first
check of the function, and continue using this variable instead of
performing array accesses again in this function.
>
>> @@ -516,14 +517,22 @@ int evtchn_fifo_init_control(struct evtchn_init_control 
>> *init_control)
>>      gfn     = init_control->control_gfn;
>>      offset  = init_control->offset;
>>  
>> -    if ( vcpu_id >= d->max_vcpus || !d->vcpu[vcpu_id] )
>> +    if ( !domain_vcpu(d, vcpu_id) )
>>          return -ENOENT;
>> -    v = d->vcpu[vcpu_id];
>> +
>> +    v = domain_vcpu(d, vcpu_id);
> Please don't call the function twice.

I will assign the variable as part of the if statement.

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