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

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

[Andrew Cooper]

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.

Drop the !sve check entirely.

Signed-off-by: Andrew Cooper <andrew.cooper3@citrix.com>
---
CC: Razvan Cojocaru <rcojocaru@bitdefender.com>
CC: Tamas K Lengyel <tamas@tklengyel.com>
CC: Jun Nakajima <jun.nakajima@intel.com>
CC: Kevin Tian <kevin.tian@intel.com>
CC: Jan Beulich <JBeulich@suse.com>
CC: Wei Liu <wei.liu2@citrix.com>
CC: Roger Pau Monné <roger.pau@citrix.com>
CC: Juergen Gross <jgross@suse.com>

Discovered while trying to fix the gaping security hole with ballooning out
the #VE info page.  The risk for 4.12 is very minimal - altp2m is off by
default, not security supported, and the ability to clearing sve is limited to
introspection code paths.
---
 xen/arch/x86/mm/p2m-ept.c | 10 ----------
 1 file changed, 10 deletions(-)

diff --git a/xen/arch/x86/mm/p2m-ept.c b/xen/arch/x86/mm/p2m-ept.c
index 2b2bf31..bb56260 100644
--- 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;
-    }
-
     /*
      * the caller must make sure:
      * 1. passing valid gfn and mfn at order boundary.
-- 
2.1.4


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

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

[Razvan Cojocaru]

On 1/24/19 8:28 PM, Andrew Cooper 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.
> 
> Drop the !sve check entirely.
> 
> Signed-off-by: Andrew Cooper <andrew.cooper3@citrix.com>
> ---
> CC: Razvan Cojocaru <rcojocaru@bitdefender.com>
> CC: Tamas K Lengyel <tamas@tklengyel.com>
> CC: Jun Nakajima <jun.nakajima@intel.com>
> CC: Kevin Tian <kevin.tian@intel.com>
> CC: Jan Beulich <JBeulich@suse.com>
> CC: Wei Liu <wei.liu2@citrix.com>
> CC: Roger Pau Monné <roger.pau@citrix.com>
> CC: Juergen Gross <jgross@suse.com>
> 
> Discovered while trying to fix the gaping security hole with ballooning out
> the #VE info page.  The risk for 4.12 is very minimal - altp2m is off by
> default, not security supported, and the ability to clearing sve is limited to
> introspection code paths.

Reviewed-by: Razvan Cojocaru <rcojocaru@bitdefender.com>


Thanks,
Razvan

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

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

[Juergen Gross]

On 24/01/2019 19:28, Andrew Cooper 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.
> 
> Drop the !sve check entirely.
> 
> Signed-off-by: Andrew Cooper <andrew.cooper3@citrix.com>

Release-acked-by: Juergen Gross <jgross@suse.com>


Juergen

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

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

[Jan Beulich]

>>> On 24.01.19 at 19:28, <andrew.cooper3@citrix.com> 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?

> condition is also wrong.

What about the similar conditions in the handling of
HVMOP_altp2m_vcpu_{en,dis}able_notify then?

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

Jan



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

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

[Andrew Cooper]

On 25/01/2019 10:25, Jan Beulich wrote:
>>>> On 24.01.19 at 19:28, <andrew.cooper3@citrix.com> 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@lists.xenproject.org
https://lists.xenproject.org/mailman/listinfo/xen-devel

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

[Jan Beulich]

>>> On 25.01.19 at 12:10, <andrew.cooper3@citrix.com> wrote:
> On 25/01/2019 10:25, Jan Beulich wrote:
>>>>> On 24.01.19 at 19:28, <andrew.cooper3@citrix.com> 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; 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),
- the third paragraph is about the other condition, which I
  agree should go away.
I'm sorry for being dense, but I seem to need a more explicit
explanation.

Jan



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

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

[Andrew Cooper]

On 25/01/2019 13:25, Jan Beulich wrote:
>>>> On 25.01.19 at 12:10, <andrew.cooper3@citrix.com> wrote:
>> On 25/01/2019 10:25, Jan Beulich wrote:
>>>>>> On 24.01.19 at 19:28, <andrew.cooper3@citrix.com> 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@lists.xenproject.org
https://lists.xenproject.org/mailman/listinfo/xen-devel

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

[Jan Beulich]

>>> On 25.01.19 at 15:15, <andrew.cooper3@citrix.com> wrote:
> On 25/01/2019 13:25, Jan Beulich wrote:
>>>>> On 25.01.19 at 12:10, <andrew.cooper3@citrix.com> wrote:
>>> On 25/01/2019 10:25, Jan Beulich wrote:
>>>>>>> On 24.01.19 at 19:28, <andrew.cooper3@citrix.com> 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@suse.com>

Jan


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

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

[Tian, Kevin]

> From: Andrew Cooper [mailto:andrew.cooper3@citrix.com]
> Sent: Friday, January 25, 2019 2:28 AM
> 
> 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.
> 
> Drop the !sve check entirely.
> 
> Signed-off-by: Andrew Cooper <andrew.cooper3@citrix.com>

Reviewed-by: Kevin Tian <kevin.tian@intel.com>
_______________________________________________
Xen-devel mailing list
Xen-devel@lists.xenproject.org
https://lists.xenproject.org/mailman/listinfo/xen-devel