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

["Jan Beulich" <JBeulich@xxxxxxxx>]

>>> On 25.01.19 at 15:15, <andrew.cooper3@xxxxxxxxxx> wrote:
> On 25/01/2019 13:25, Jan Beulich wrote:
>>>>> On 25.01.19 at 12:10, <andrew.cooper3@xxxxxxxxxx> wrote:
>>> 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
>>>>> condition is also wrong.
>>>>> [...]
>>>>> --- 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.
>> I'm struggling. For clarity I've retained all of the relevant parts
>> of the description in context above. I can't identify where you
>> talk about an out of bounds read there. So
>> - for the first paragraph, acting upon the wrong side of the union
>>   does not apply with the added qualifier,
>> - for the second paragraph, the domain itself requesting an action
>>   upon something not fully initialized yet looks to deserve an error
>>   return;
> 
> The issue is here.  Setting up EPT.SVE is largely unrelated to setting
> up set delivery of #VE.
> 
> This is like asking "can I execute an `int3` instruction before setting
> up an IDT?"  Sure - it's not a clever idea to try, but you don't get
> back an -EINVAL for trying to execute `int3` before `lidt`.
> 
> Furthermore, even with this interlock in place, it doesn't guarantee
> that #VE delivery will work - there are further in-guest steps required
> not to end up with #DF.
> 
>>   it looks wrong to me to request #VE without first setting
>>   up the page needed for its proper delivery (I'd even question
>>   the validity of doing so by a controlling domain, but I accept that
>>   we can't work out the correct vCPU to check - perhaps the right
>>   thing would be to check all of them, as the P2M is per-domain,
>>   not per-vCPU),
> 
> Redirecting #VE internally is an optimisation over causing an
> EPT_VIOLATION vmexit.  One of these two will happen.
> 
> As it turns out, some corner cases will VMExit anyway, so its not
> actually safe for a guest to use this infrastructure on itself, without
> an external introspection agent.
> 
> It is definitely not acceptable to prevent an external agent from
> configuring EPT head of time, so its internal agent can take over
> handling of #VE when it is ready.

Well, my question was really only whether to deny the operation for
a guest on itself.

> So, overall, the only thing this check does is force the ordering of two
> startup actions (which are fine to mis-order if you know what you are
> doing) inside a Xen-aware in-guest agent, but doesn't interact with a
> number of related things which can ultimately lead to a guest crash.
> 
> Or, in other words, it is simply not a useful thing to enforce.

Okay, having looked some more at the SDM I see that #VE can't
really occur without the veinfo_gfn first having got set up (and
stored in the VMCS), as there's a respective VM entry check
disallowing the enable bit to be set without a valid address. IOW
I agree now with this last statement of yours:
Reviewed-by: Jan Beulich <jbeulich@xxxxxxxx>

Jan


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