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

[Norbert Manthey <nmanthey@xxxxxxxxx>]

On 2/1/19 15:08, Jan Beulich wrote:
>>>> On 01.02.19 at 14:45, <nmanthey@xxxxxxxxx> wrote:
>> On 1/31/19 16:05, Jan Beulich wrote:
>>>>>> On 29.01.19 at 15:43, <nmanthey@xxxxxxxxx> 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.
> I don't see how an overflow into the valid range could occur: Negative
> numbers, when converted to unsigned, become large positive numbers.
> If anything in this regard was to change here, then the type of _both_
> local variable (which get initialized from a field of type uint32_t).
True, I will drop the < 0 check as well.
>
>>>> @@ -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.
> But domain_vcpu() does not alter an out of bounds value passed
> into it in any way, i.e. subsequent array accesses using that value
> would still be an issue. IOW in the case here what you do is
> sufficient because there's no array access in the first place. It's
> debatable whether any change is needed at all here (there would
> need to be a speculation path which could observe the result of
> the speculative write into chn->notify_vcpu_id).

In this method, the access to d->vcpu[vcpu] has to be protected. That
happens by using the domain_vcpu function. The rest of this function
does not read the vcpu variable, as you mentioned. Therefore, I would
keep this version of the fix, and also drop the sign check as above.

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