Re: [Xen-devel] [PATCH] xen/sched: Introduce domain_vcpu() helper

["Jan Beulich" <JBeulich@xxxxxxxx>]

>>> On 23.01.19 at 18:44, <andrew.cooper3@xxxxxxxxxx> wrote:
> On 23/01/2019 17:01, Jan Beulich wrote:
>>>>> On 23.01.19 at 15:59, <andrew.cooper3@xxxxxxxxxx> 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@xxxxxxxxxxxxxxxxxxxx
https://lists.xenproject.org/mailman/listinfo/xen-devel