Re: [Xen-devel] [PATCH for-4.12] x86/p2m: Drop erroneous #VE-enabled check in ept_set_entry()
> From: Andrew Cooper [mailto:andrew.coop...@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 Reviewed-by: Kevin Tian ___ Xen-devel mailing list Xen-devel@lists.xenproject.org https://lists.xenproject.org/mailman/listinfo/xen-devel
Re: [Xen-devel] [PATCH for-4.12] x86/p2m: Drop erroneous #VE-enabled check in ept_set_entry()
>>> On 25.01.19 at 15:15, wrote: > On 25/01/2019 13:25, Jan Beulich wrote: > On 25.01.19 at 12:10, wrote: >>> On 25/01/2019 10:25, Jan Beulich wrote: >>> On 24.01.19 at 19:28, 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 Jan ___ Xen-devel mailing list Xen-devel@lists.xenproject.org https://lists.xenproject.org/mailman/listinfo/xen-devel
Re: [Xen-devel] [PATCH for-4.12] x86/p2m: Drop erroneous #VE-enabled check in ept_set_entry()
On 25/01/2019 13:25, Jan Beulich wrote: On 25.01.19 at 12:10, wrote: >> On 25/01/2019 10:25, Jan Beulich wrote: >> On 24.01.19 at 19:28, 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: [Xen-devel] [PATCH for-4.12] x86/p2m: Drop erroneous #VE-enabled check in ept_set_entry()
>>> On 25.01.19 at 12:10, wrote: > On 25/01/2019 10:25, Jan Beulich wrote: > On 24.01.19 at 19:28, 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: [Xen-devel] [PATCH for-4.12] x86/p2m: Drop erroneous #VE-enabled check in ept_set_entry()
On 25/01/2019 10:25, Jan Beulich wrote: On 24.01.19 at 19:28, 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: [Xen-devel] [PATCH for-4.12] x86/p2m: Drop erroneous #VE-enabled check in ept_set_entry()
>>> On 24.01.19 at 19:28, 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: [Xen-devel] [PATCH for-4.12] x86/p2m: Drop erroneous #VE-enabled check in ept_set_entry()
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 Release-acked-by: Juergen Gross Juergen ___ Xen-devel mailing list Xen-devel@lists.xenproject.org https://lists.xenproject.org/mailman/listinfo/xen-devel
Re: [Xen-devel] [PATCH for-4.12] x86/p2m: Drop erroneous #VE-enabled check in ept_set_entry()
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 > --- > CC: Razvan Cojocaru > CC: Tamas K Lengyel > CC: Jun Nakajima > CC: Kevin Tian > CC: Jan Beulich > CC: Wei Liu > CC: Roger Pau Monné > CC: Juergen Gross > > 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 Thanks, Razvan ___ Xen-devel mailing list Xen-devel@lists.xenproject.org https://lists.xenproject.org/mailman/listinfo/xen-devel
[Xen-devel] [PATCH for-4.12] x86/p2m: Drop erroneous #VE-enabled check in ept_set_entry()
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 --- CC: Razvan Cojocaru CC: Tamas K Lengyel CC: Jun Nakajima CC: Kevin Tian CC: Jan Beulich CC: Wei Liu CC: Roger Pau Monné CC: Juergen Gross 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