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

["Jan Beulich" <JBeulich@xxxxxxxx>]

>>> On 24.01.19 at 13:33, <andrew.cooper3@xxxxxxxxxx> wrote:
> On 24/01/2019 08:35, Jan Beulich wrote:
>>>>> 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.
> 
> Oops yes.  You're right that is a real non-speculative issue here.
> 
> The correct code is:
> 
> {
>     unsigned int idx = array_index_nospec(vcpu_id, d->max_vcpus);
> 
>     return vcpu_id >= d->max_vcpus ? NULL : d->vcpu[idx];
> }
> 
> Which will return a real NULL for all non-speculative out-of-bounds
> requests, and will return d->vcpu[0] during incorrect speculation.

And in this shape it's then
Reviewed-by: Jan Beulich <jbeulich@xxxxxxxx>

Jan



_______________________________________________
Xen-devel mailing list
Xen-devel@xxxxxxxxxxxxxxxxxxxx
https://lists.xenproject.org/mailman/listinfo/xen-devel