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

["Jan Beulich" <JBeulich@suse.com>]

>>> On 01.02.19 at 14:45, <nmanthey@amazon.de> wrote:
> 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.

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).

>>> @@ -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).

Jan



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