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


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.

~Andrew

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