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