Re: [Xen-devel] [PATCH for-4.12] x86/p2m: Drop erroneous #VE-enabled check in ept_set_entry()

[Andrew Cooper <andrew.cooper3@xxxxxxxxxx>]

On 25/01/2019 10:25, Jan Beulich wrote:
>>>> On 24.01.19 at 19:28, <andrew.cooper3@xxxxxxxxxx> wrote:
>> Code clearing the "Suppress VE" bit in an EPT entry isn't nececsserily 
>> running
>> in current context.  In ALTP2M_external mode, it definitely is not, and in PV
>> context, vcpu_altp2m(current) acts upon the HVM union.
>>
>> Even if we could sensibly resolve the target vCPU, it may legitimately not be
>> fully set up at this point, so rejecting the EPT modification would be buggy.
>>
>> There is a path in hvm_hap_nested_page_fault() which explicitly emulates #VE
>> in the cpu_has_vmx_virt_exceptions case, so the -EOPNOTSUPP part of this
> ITYM "in the !cpu_has_vmx_virt_exceptions case" here?

I do, yes.

>
>> condition is also wrong.
> What about the similar conditions in the handling of
> HVMOP_altp2m_vcpu_{en,dis}able_notify then?

In hindsight, I think that restriction (which was after the fact) was a
poor choice.  I certainly wasn't aware of the emulated support at the time.

>
>> --- a/xen/arch/x86/mm/p2m-ept.c
>> +++ b/xen/arch/x86/mm/p2m-ept.c
>> @@ -702,16 +702,6 @@ ept_set_entry(struct p2m_domain *p2m, gfn_t gfn_, mfn_t 
>> mfn,
>>  
>>      ASSERT(ept);
>>  
>> -    if ( !sve )
>> -    {
>> -        if ( !cpu_has_vmx_virt_exceptions )
>> -            return -EOPNOTSUPP;
>> -
>> -        /* #VE should be enabled for this vcpu. */
>> -        if ( gfn_eq(vcpu_altp2m(current).veinfo_gfn, INVALID_GFN) )
>> -            return -ENXIO;
>> -    }
> How about retaining the latter, but qualifying it with
> current->domain == d?

Why?  There is a paragraph in the commit message explaining why this
check is wrong even when it isn't an out-of-bounds read.

~Andrew

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