>>> On 23.01.19 at 18:44, <andrew.coop...@citrix.com> wrote: > On 23/01/2019 17:01, Jan Beulich wrote: >>>>> On 23.01.19 at 15:59, <andrew.coop...@citrix.com> wrote: >>> +static inline struct vcpu *domain_vcpu(const struct domain *d, >>> + unsigned int vcpu_id) >>> +{ >>> + unsigned int idx = array_index_nospec(vcpu_id, d->max_vcpus); >>> + >>> + return idx >= d->max_vcpus ? NULL : d->vcpu[idx]; >>> +} >> For an out of bounds incoming vcpu_id, isn't it the case that >> idx then would be zero? In which case you'd return d->vcpu[0] >> instead of NULL? > > Speculatively, yes. array_index_nospec() works by forcing speculative > mis-accesses to operate as if it request had been for index 0. > > What matters from a data-leaking perspective is whether d->vcpu[idx], > when executed speculative, ends up being out-of-bounds or not. i.e. > whether it is distinguishable from a path which can architecturally be > taken.
I'm afraid we're talking of different aspects. I'm not considering the speculation aspect at all, but the mere base functionality. A caller of this is, I think, supposed to check whether it got back NULL in order to know whether the passed in vCPU ID was valid. If non-NULL, it ought to act on that vCPU. But you don't want invalid vCPU IDs getting passed into hypercalls to result in action on vCPU 0, do you? Or are you implying that callers ought to duplicate the bounds checking done here? If so, this would need to be called out very prominently in the comment accompanying the function, to hopefully avoid anyone stepping into this trap. > P.S. index 0 is actually better than NULL on any hardware lacking SMAP, > because you won't potentially use guest-controlled data from 0 during > the subsequent speculation. Is that the case in the way you describe it? I thought one of the base issues with some of last year's speculation issues was that data related #PF get evaluated only at the end of the pipeline, when retiring insns. To me this would imply speculation through NULL is equally happening with SMAP. Furthermore 32-bit PV guests could place a kernel mapping there. Of course the implication would be that avoiding to hand back NULL has even wider benefit. But then the question is whether handing back NULL here and elsewhere shouldn't be avoided altogether. Jan _______________________________________________ Xen-devel mailing list Xen-devel@lists.xenproject.org https://lists.xenproject.org/mailman/listinfo/xen-devel