On 1/24/19 17:56, Jan Beulich wrote: >>>> On 23.01.19 at 12:57, <nmant...@amazon.de> wrote: >> --- a/xen/common/event_channel.c >> +++ b/xen/common/event_channel.c >> @@ -368,8 +368,14 @@ int evtchn_bind_virq(evtchn_bind_virq_t *bind, >> evtchn_port_t port) >> if ( virq_is_global(virq) && (vcpu != 0) ) >> 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)); > I think this wants to move ahead of the if() in context, to be independent > of the particular implementation of virq_is_global() (the current shape of > which is mostly fine, perhaps with the exception of the risk of the compiler > translating the switch() there by way of a jump table). This also moves it > closer to the if() the construct is a companion to. I understand the concern. However, because the value of virq would be changed before the virq_is_global check, couldn't that result in returning a wrong error code? The potential out-of-bound value is brought back into the valid range, so that the above check might fire incorrectly? > >> @@ -816,6 +822,12 @@ int set_global_virq_handler(struct domain *d, uint32_t >> virq) >> if (!virq_is_global(virq)) >> return -EINVAL; >> >> + /* >> + * Make sure the guest controlled value virq is bounded even during >> + * speculative execution. >> + */ >> + virq = array_index_nospec(virq, ARRAY_SIZE(global_virq_handlers)); > Same here then. > >> @@ -931,7 +943,8 @@ long evtchn_bind_vcpu(unsigned int port, unsigned int >> vcpu_id) >> struct evtchn *chn; >> long rc = 0; >> >> - if ( (vcpu_id >= d->max_vcpus) || (d->vcpu[vcpu_id] == NULL) ) >> + if ( (vcpu_id >= d->max_vcpus) || >> + (d->vcpu[array_index_nospec(vcpu_id, d->max_vcpus)] == NULL) ) >> return -ENOENT; >> >> spin_lock(&d->event_lock); >> @@ -969,8 +982,10 @@ 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(d->vcpu[array_index_nospec(vcpu_id, >> + >> d->max_vcpus)]->processor)); >> + link_pirq_port(port, chn, d->vcpu[array_index_nospec(vcpu_id, >> + >> d->max_vcpus)]); > Using Andrew's new domain_vcpu() will improve readability, especially > after your change, quite a bit here. But of course code elsewhere will > benefit as well.
You mean I should use the domain_vcpu function in both hunks, because due to the first one, the latter can never return NULL? I will rebase the series on top of this fresh change, and use the domain_vcpu function for the locations where I bound a vcpu_id. 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
