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

[Alexandru Stefan ISAILA <aisaila@xxxxxxxxxxxxxxx>]

On 17.12.2019 18:50, Jan Beulich wrote:
> On 17.12.2019 16:12, Alexandru Stefan ISAILA wrote:
>> --- a/xen/arch/x86/mm/mem_access.c
>> +++ b/xen/arch/x86/mm/mem_access.c
>> @@ -367,10 +367,11 @@ long p2m_set_mem_access(struct domain *d, gfn_t gfn, 
>> uint32_t nr,
>>       if ( altp2m_idx )
>>       {
>>           if ( altp2m_idx >= MAX_ALTP2M ||
>> -             d->arch.altp2m_eptp[altp2m_idx] == mfn_x(INVALID_MFN) )
>> +             d->arch.altp2m_eptp[array_index_nospec(altp2m_idx, MAX_EPTP)] 
>> ==
> 
> The bounds check is against MAX_ALTP2M. Both MAX_ values look to be
> independent, which means bounds check and value passed to the
> helper need to match up (not just here).

I will have both checks against MAX_ALTP2M.

> 
>> --- a/xen/arch/x86/mm/p2m-ept.c
>> +++ b/xen/arch/x86/mm/p2m-ept.c
>> @@ -1353,7 +1353,8 @@ void setup_ept_dump(void)
>>   
>>   void p2m_init_altp2m_ept(struct domain *d, unsigned int i)
>>   {
>> -    struct p2m_domain *p2m = d->arch.altp2m_p2m[i];
>> +    struct p2m_domain *p2m =
>> +           d->arch.altp2m_p2m[array_index_nospec(i, MAX_ALTP2M)];
>>       struct p2m_domain *hostp2m = p2m_get_hostp2m(d);
>>       struct ept_data *ept;
>>   
>> @@ -1366,7 +1367,7 @@ void p2m_init_altp2m_ept(struct domain *d, unsigned 
>> int i)
>>       p2m->max_mapped_pfn = p2m->max_remapped_gfn = 0;
>>       ept = &p2m->ept;
>>       ept->mfn = pagetable_get_pfn(p2m_get_pagetable(p2m));
>> -    d->arch.altp2m_eptp[i] = ept->eptp;
>> +    d->arch.altp2m_eptp[array_index_nospec(i, MAX_EPTP)] = ept->eptp;
>>   }
>>   
>>   unsigned int p2m_find_altp2m_by_eptp(struct domain *d, uint64_t eptp)
>> --- a/xen/arch/x86/mm/p2m.c
>> +++ b/xen/arch/x86/mm/p2m.c
>> @@ -2499,7 +2499,7 @@ static void p2m_reset_altp2m(struct domain *d, 
>> unsigned int idx,
>>       struct p2m_domain *p2m;
>>   
>>       ASSERT(idx < MAX_ALTP2M);
>> -    p2m = d->arch.altp2m_p2m[idx];
>> +    p2m = d->arch.altp2m_p2m[array_index_nospec(idx, MAX_ALTP2M)];
>>   
>>       p2m_lock(p2m);
>>   
>> @@ -2540,7 +2540,7 @@ static int p2m_activate_altp2m(struct domain *d, 
>> unsigned int idx)
>>   
>>       ASSERT(idx < MAX_ALTP2M);
>>   
>> -    p2m = d->arch.altp2m_p2m[idx];
>> +    p2m = d->arch.altp2m_p2m[array_index_nospec(idx, MAX_ALTP2M)];
> 
> All of the above have a more or less significant disconnect between
> the bounds check and the use as array index. I think it would be
> quite helpful if these could live close to one another, so one can
> (see further up) easily prove that both specified bounds actually
> match up.
> 

Sure, I can move the array use closer together.

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