>>> On 24.01.19 at 13:33, <andrew.coop...@citrix.com> wrote:
> On 24/01/2019 08:35, Jan Beulich wrote:
>>>>> 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.
>
> 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 <jbeul...@suse.com>
Jan
_______________________________________________
Xen-devel mailing list
Xen-devel@lists.xenproject.org
https://lists.xenproject.org/mailman/listinfo/xen-devel
