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

[Jan Beulich <jbeulich@suse.com>]

On 20.12.2019 10:09, Alexandru Stefan ISAILA wrote:
> 
> 
> On 19.12.2019 12:43, Jan Beulich wrote:
>> On 19.12.2019 10:42, Alexandru Stefan ISAILA wrote:
>>> This patch aims to sanitize indexes, potentially guest provided
>>> values, for altp2m_eptp[] and altp2m_p2m[] arrays.
>>>
>>> Requested-by: Jan Beulich <jbeulich@suse.com>
>>> Signed-off-by: Alexandru Isaila <aisaila@bitdefender.com>
>>> ---
>>> CC: Razvan Cojocaru <rcojocaru@bitdefender.com>
>>> CC: Tamas K Lengyel <tamas@tklengyel.com>
>>> CC: Petre Pircalabu <ppircalabu@bitdefender.com>
>>> CC: George Dunlap <george.dunlap@eu.citrix.com>
>>> CC: Jan Beulich <jbeulich@suse.com>
>>> CC: Andrew Cooper <andrew.cooper3@citrix.com>
>>> CC: Wei Liu <wl@xen.org>
>>> CC: "Roger Pau Monné" <roger.pau@citrix.com>
>>> CC: Jun Nakajima <jun.nakajima@intel.com>
>>> CC: Kevin Tian <kevin.tian@intel.com>
>>> ---
>>> Changes since V4:
>>> 	- Change bounds check from MAX_EPTP to MAX_ALTP2M
>>> 	- Move array_index_nospec() closer to the bounds check.
>>> ---
>>>   xen/arch/x86/mm/mem_access.c | 15 +++++++++------
>>>   xen/arch/x86/mm/p2m.c        | 20 ++++++++++++++------
>>>   2 files changed, 23 insertions(+), 12 deletions(-)
>>>
>>> diff --git a/xen/arch/x86/mm/mem_access.c b/xen/arch/x86/mm/mem_access.c
>>> index 320b9fe621..33e379db8f 100644
>>> --- 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 ||
> 
> Ok, so have if ( altp2m_idx >=  min(ARRAY_SIZE(d->arch.altp2m_eptp), 
> MAX_EPTP) ||
> here and then...
> 
>>> -             d->arch.altp2m_eptp[altp2m_idx] == mfn_x(INVALID_MFN) )
>>> +             d->arch.altp2m_eptp[array_index_nospec(altp2m_idx, MAX_ALTP2M)] ==
> 
> have MAX_EPTP here and ...
> 
>>
>> As implied by a reply to v4, this is still latently buggy: There's
>> no guarantee anyone will notice the issue here when bumping
>> MAX_ALTP2M past MAX_EPTP. The only future proof thing to do here
>> is, as suggested, using some form of min(MAX_ALTP2M, MAX_EPTP) in
>> the actual bounds check. Then each array access itself can be made
>> use the correct bound. In fact you'd probably have noticed this if
>> you had made use of array_access_nospec() where possible (which
>> also would help readability) - apparently not 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)];
> 
> MAX_ALTP2M here ?

Yes, that's how I think it ought to be. Give others a chance to
disagree with me, though.

Jan

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