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

[Alexandru Stefan ISAILA <aisaila@xxxxxxxxxxxxxxx>]

On 18.12.2019 12:06, Jan Beulich wrote:
> On 18.12.2019 10:57, Alexandru Stefan ISAILA wrote:
>> On 18.12.2019 10:06, Alexandru Stefan ISAILA wrote:
>>> On 17.12.2019 18:50, Jan Beulich wrote:
>>>> On 17.12.2019 16:12, Alexandru Stefan ISAILA wrote:
>>>>> --- 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.
>>>
>>
>> Sorry to come back on this but I was looking in the code and I am not
>> sure I follow where is the disconnect. If you are talking about
>> p2m_init_altp2m_ept() the eptp code will move up in patch 3/4.
> 
> My remark was about all four hunks left in context (and then still
> possibly extending to other ones). Let's take the last one above:
> p2m_activate_altp2m() has two callers, one of which loops over
> altp2m-s (and hence doesn't need the guard). The other one is
> p2m_init_altp2m_by_id() which does the range check I'm talking
> about (ASSERT() doesn't count), and which therefore is the place
> to use array_index_nospec(). Once you look there you'll notice
> that the function also has an array access itself which you've
> left untouched.
> 

So add a "idx = array_index_nospec(idx, MAX_ALTP2M)" in the callers 
where there is a need for this and drop checks in the lower functions.


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