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

From: Alexandru Stefan ISAILA <aisaila@bitdefender.com>
To: Jan Beulich <jbeulich@suse.com>
Cc: "Petre Ovidiu PIRCALABU" <ppircalabu@bitdefender.com>,
	"Kevin Tian" <kevin.tian@intel.com>,
	"Tamas K Lengyel" <tamas@tklengyel.com>, "Wei Liu" <wl@xen.org>,
	"Razvan COJOCARU" <rcojocaru@bitdefender.com>,
	"George Dunlap" <george.dunlap@eu.citrix.com>,
	"Andrew Cooper" <andrew.cooper3@citrix.com>,
	"Jun Nakajima" <jun.nakajima@intel.com>,
	"xen-devel@lists.xenproject.org" <xen-devel@lists.xenproject.org>,
	"Roger Pau Monné" <roger.pau@citrix.com>
Subject: Re: [Xen-devel] [PATCH V4 1/4] x86/mm: Add array_index_nospec to guest provided index values
Date: Wed, 18 Dec 2019 09:57:01 +0000	[thread overview]
Message-ID: <131f196c-e149-3cec-765b-be7bf36bf19b@bitdefender.com> (raw)
In-Reply-To: <20fedb38-ac6e-772b-1859-eebd03c92989@bitdefender.com>

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/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.
> 

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.

Can you please clarify?

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