On 2/1/19 15:08, Jan Beulich wrote: >>>> On 01.02.19 at 14:45, <nmant...@amazon.de> wrote: >> On 1/31/19 16:05, Jan Beulich wrote: >>>>>> On 29.01.19 at 15:43, <nmant...@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). 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@lists.xenproject.org https://lists.xenproject.org/mailman/listinfo/xen-devel
