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

[Alexandru Stefan ISAILA <aisaila@xxxxxxxxxxxxxxx>]

On 27.12.2019 10:01, Jan Beulich wrote:
> (re-sending, as I still don't see the mail having appeared on the list)
> 
> On 23.12.2019 15:04, Alexandru Stefan ISAILA wrote:
>> Changes since V5:
>>      - Add black lines
> 
> Luckily no color comes through in plain text mails ;-)
> 
>> --- 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?


> 
>> @@ -425,11 +426,12 @@ long p2m_set_mem_access_multi(struct domain *d,
>>   #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) ||
>> +             d->arch.altp2m_eptp[array_index_nospec(altp2m_idx, MAX_EPTP)] 
>> ==
>> +             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)];
> 
> Same two remarks here then, and again further down.
> 
>> --- a/xen/arch/x86/mm/p2m.c
>> +++ b/xen/arch/x86/mm/p2m.c
>> @@ -2577,6 +2577,8 @@ int p2m_init_altp2m_by_id(struct domain *d, unsigned 
>> int idx)
>>       if ( idx >= MAX_ALTP2M )
>>           return rc;
>>   
>> +    idx = array_index_nospec(idx, MAX_ALTP2M);
>> +
>>       altp2m_list_lock(d);
>>   
>>       if ( d->arch.altp2m_eptp[idx] == mfn_x(INVALID_MFN) )
> 
> What about this array access?
> 
>> @@ -2618,6 +2620,8 @@ int p2m_destroy_altp2m_by_id(struct domain *d, 
>> unsigned int idx)
>>       if ( !idx || idx >= MAX_ALTP2M )
>>           return rc;
>>   
>> +    idx = array_index_nospec(idx, MAX_ALTP2M);
> 
> There's a d->arch.altp2m_eptp[] access down from here too. I'm not
> going to look further. Please get things into consistent shape while
> you do this transformation.
> 

I will change the idx part in p2m_init_altp2m_by_id() and 
p2m_destroy_altp2m_by_id() so they match the rest of the checks:
"if ( idx >=  min(ARRAY_SIZE(d->arch.altp2m_p2m), MAX_EPTP))...", drop 
the idx = array_index_nospec(idx, MAX_ALTP2M); and have 
array_index_nospec() into place.


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