Re: [Xen-devel] [PATCH V6 1/4] x86/mm: Add array_index_nospec to guest provided index values

[Jan Beulich <jbeulich@xxxxxxxx>]

On 07.01.2020 15:31, Alexandru Stefan ISAILA wrote:
> 
> 
> On 07.01.2020 15:55, Jan Beulich wrote:
>> On 07.01.2020 14:25, Alexandru Stefan ISAILA wrote:
>>> On 27.12.2019 10:01, Jan Beulich wrote:
>>>> On 23.12.2019 15:04, Alexandru Stefan ISAILA wrote:
>>>>> --- a/xen/arch/x86/mm/mem_access.c
>>>>> +++ b/xen/arch/x86/mm/mem_access.c
>>>>> @@ -366,11 +366,12 @@ long p2m_set_mem_access(struct domain *d, gfn_t 
>>>>> gfn, uint32_t nr,
>>>>>    #ifdef CONFIG_HVM
>>>>>        if ( altp2m_idx )
>>>>>        {
>>>>> -        if ( altp2m_idx >= MAX_ALTP2M ||
>>>>> -             d->arch.altp2m_eptp[altp2m_idx] == mfn_x(INVALID_MFN) )
>>>>> +        if ( altp2m_idx >=  min(ARRAY_SIZE(d->arch.altp2m_p2m), 
>>>>> MAX_EPTP) ||
>>>>
>>>> Stray blank after >= .
>>>>
>>>>> +             d->arch.altp2m_eptp[array_index_nospec(altp2m_idx, 
>>>>> MAX_EPTP)] ==
>>>>
>>>> I accept you can't (currently) use array_access_nospec() here,
>>>> but ...
>>>>
>>>>> +             mfn_x(INVALID_MFN) )
>>>>>                return -EINVAL;
>>>>>    
>>>>> -        ap2m = d->arch.altp2m_p2m[altp2m_idx];
>>>>> +        ap2m = d->arch.altp2m_p2m[array_index_nospec(altp2m_idx, 
>>>>> MAX_ALTP2M)];
>>>>
>>>> ... I don't see why you still effectively open-code it here.
>>>
>>> I am not sure I follow you here, that is what we agreed in v5
>>> (https://lists.xenproject.org/archives/html/xen-devel/2019-12/msg01704.html).
>>> Did I miss something?
>>
>> In context there (from an earlier reply of mine) you will find me
>> having mentioned array_access_nospec(). This wasn't invalidated or
>> overridden by my "Yes, that's how I think it ought to be." I didn't
>> say so explicitly (again) because to me it goes without saying that
>> open-coding _anything_ is, in the common case, bad practice.
>>
> 
> So the way to go is to have:
> 
> altp2m_idx = array_index_nospec(altp2m_idx, MAX_ALTP2M);
> ap2m = d->arch.altp2m_p2m[altp2m_idx];

No. The way to go is to use array_access_nospec() wherever possible.
Besides (as said) avoiding its open-coding, this is the construct
correctly matching your uses of ARRAY_SIZE(), avoiding the explicit
specification of the upper array bound (MAX_ALTP2M). (I really don't
see how my previous reply was not crystal clear in this regard.)

Jan

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