[PATCH 0/2] KVM: VMX: Fixes for faults on ENCLS emulation

[PATCH 0/2] KVM: VMX: Fixes for faults on ENCLS emulation

[Sean Christopherson]

Found-by-inspection (when reviewing Binbin's patch) fixes for incorrect
emulation of faults when KVMintercepts and emulates (sort of) ENCLS.

Very much compile tested only.  Ideally, someone with SGX hardware can
confirm that these patches are correct, e.g. my assessment that KVM needs
to manually check CR0.PG is based purely of SDM pseudocode.

Sean Christopherson (2):
  KVM: VMX: Inject #GP on ENCLS if vCPU has paging disabled (CR0.PG==0)
  KVM: VMX: Inject #GP, not #UD, if SGX2 ENCLS leafs are unsupported

 arch/x86/kvm/vmx/sgx.c | 15 +++++++++------
 1 file changed, 9 insertions(+), 6 deletions(-)


base-commit: 27d6845d258b67f4eb3debe062b7dacc67e0c393
-- 
2.40.0.348.gf938b09366-goog

[PATCH 1/2] KVM: VMX: Inject #GP on ENCLS if vCPU has paging disabled (CR0.PG==0)

[Sean Christopherson]

Inject a #GP when emulating/forwarding a valid ENCLS leaf if the vCPU has
paging disabled, e.g. if KVM is intercepting ECREATE to enforce additional
restrictions.  The pseudocode in the SDM lists all #GP triggers, including
CR0.PG=0, as being checked after the ENLCS-exiting checks, i.e. the
VM-Exit will occur before the CPU performs the CR0.PG check.

Fixes: 70210c044b4e ("KVM: VMX: Add SGX ENCLS[ECREATE] handler to enforce CPUID restrictions")
Cc: Binbin Wu <binbin.wu@linux.intel.com>
Cc: Kai Huang <kai.huang@intel.com>
Signed-off-by: Sean Christopherson <seanjc@google.com>
---
 arch/x86/kvm/vmx/sgx.c | 2 +-
 1 file changed, 1 insertion(+), 1 deletion(-)

diff --git a/arch/x86/kvm/vmx/sgx.c b/arch/x86/kvm/vmx/sgx.c
index aa53c98034bf..f881f6ff6408 100644
--- a/arch/x86/kvm/vmx/sgx.c
+++ b/arch/x86/kvm/vmx/sgx.c
@@ -375,7 +375,7 @@ int handle_encls(struct kvm_vcpu *vcpu)
 
 	if (!encls_leaf_enabled_in_guest(vcpu, leaf)) {
 		kvm_queue_exception(vcpu, UD_VECTOR);
-	} else if (!sgx_enabled_in_guest_bios(vcpu)) {
+	} else if (!sgx_enabled_in_guest_bios(vcpu) || !is_paging(vcpu)) {
 		kvm_inject_gp(vcpu, 0);
 	} else {
 		if (leaf == ECREATE)
-- 
2.40.0.348.gf938b09366-goog

[PATCH 2/2] KVM: VMX: Inject #GP, not #UD, if SGX2 ENCLS leafs are unsupported

[Sean Christopherson]

Per Intel's SDM, unsupported ENCLS leafs result in a #GP, not a #UD.
SGX1 is a special snowflake as the SGX1 flag is used by the CPU as a
"soft" disable, e.g. if software disables machine check reporting, i.e.
having SGX but not SGX1 is effectively "SGX completely unsupported" and
and thus #UDs.

Fixes: 9798adbc04cf ("KVM: VMX: Frame in ENCLS handler for SGX virtualization")
Cc: Binbin Wu <binbin.wu@linux.intel.com>
Cc: Kai Huang <kai.huang@intel.com>
Signed-off-by: Sean Christopherson <seanjc@google.com>
---
 arch/x86/kvm/vmx/sgx.c | 15 +++++++++------
 1 file changed, 9 insertions(+), 6 deletions(-)

diff --git a/arch/x86/kvm/vmx/sgx.c b/arch/x86/kvm/vmx/sgx.c
index f881f6ff6408..1c092ab89c33 100644
--- a/arch/x86/kvm/vmx/sgx.c
+++ b/arch/x86/kvm/vmx/sgx.c
@@ -350,11 +350,12 @@ static int handle_encls_einit(struct kvm_vcpu *vcpu)
 
 static inline bool encls_leaf_enabled_in_guest(struct kvm_vcpu *vcpu, u32 leaf)
 {
-	if (!enable_sgx || !guest_cpuid_has(vcpu, X86_FEATURE_SGX))
-		return false;
-
+	/*
+	 * ENCLS #UDs if SGX1 isn't supported, i.e. this point will be reached
+	 * if and only if the SGX1 leafs are enabled.
+	 */
 	if (leaf >= ECREATE && leaf <= ETRACK)
-		return guest_cpuid_has(vcpu, X86_FEATURE_SGX1);
+		return true;
 
 	if (leaf >= EAUG && leaf <= EMODT)
 		return guest_cpuid_has(vcpu, X86_FEATURE_SGX2);
@@ -373,9 +374,11 @@ int handle_encls(struct kvm_vcpu *vcpu)
 {
 	u32 leaf = (u32)kvm_rax_read(vcpu);
 
-	if (!encls_leaf_enabled_in_guest(vcpu, leaf)) {
+	if (!enable_sgx || !guest_cpuid_has(vcpu, X86_FEATURE_SGX) ||
+	    !guest_cpuid_has(vcpu, X86_FEATURE_SGX1)) {
 		kvm_queue_exception(vcpu, UD_VECTOR);
-	} else if (!sgx_enabled_in_guest_bios(vcpu) || !is_paging(vcpu)) {
+	} else if (!encls_leaf_enabled_in_guest(vcpu, leaf) ||
+		   !sgx_enabled_in_guest_bios(vcpu) || !is_paging(vcpu)) {
 		kvm_inject_gp(vcpu, 0);
 	} else {
 		if (leaf == ECREATE)
-- 
2.40.0.348.gf938b09366-goog

Re: [PATCH 1/2] KVM: VMX: Inject #GP on ENCLS if vCPU has paging disabled (CR0.PG==0)

[Huang, Kai]

On Wed, 2023-04-05 at 16:45 -0700, Sean Christopherson wrote:
> Inject a #GP when emulating/forwarding a valid ENCLS leaf if the vCPU has
> paging disabled, e.g. if KVM is intercepting ECREATE to enforce additional
> restrictions.  The pseudocode in the SDM lists all #GP triggers, including
> CR0.PG=0, as being checked after the ENLCS-exiting checks, i.e. the
> VM-Exit will occur before the CPU performs the CR0.PG check.
> 
> Fixes: 70210c044b4e ("KVM: VMX: Add SGX ENCLS[ECREATE] handler to enforce CPUID restrictions")
> Cc: Binbin Wu <binbin.wu@linux.intel.com>
> Cc: Kai Huang <kai.huang@intel.com>
> Signed-off-by: Sean Christopherson <seanjc@google.com>

Reviewed-by: Kai Huang <kai.huang@intel.com>

> ---
>  arch/x86/kvm/vmx/sgx.c | 2 +-
>  1 file changed, 1 insertion(+), 1 deletion(-)
> 
> diff --git a/arch/x86/kvm/vmx/sgx.c b/arch/x86/kvm/vmx/sgx.c
> index aa53c98034bf..f881f6ff6408 100644
> --- a/arch/x86/kvm/vmx/sgx.c
> +++ b/arch/x86/kvm/vmx/sgx.c
> @@ -375,7 +375,7 @@ int handle_encls(struct kvm_vcpu *vcpu)
>  
>  	if (!encls_leaf_enabled_in_guest(vcpu, leaf)) {
>  		kvm_queue_exception(vcpu, UD_VECTOR);
> -	} else if (!sgx_enabled_in_guest_bios(vcpu)) {
> +	} else if (!sgx_enabled_in_guest_bios(vcpu) || !is_paging(vcpu)) {
>  		kvm_inject_gp(vcpu, 0);
>  	} else {
>  		if (leaf == ECREATE)
> -- 
> 2.40.0.348.gf938b09366-goog
> 

Re: [PATCH 2/2] KVM: VMX: Inject #GP, not #UD, if SGX2 ENCLS leafs are unsupported

[Huang, Kai]

On Wed, 2023-04-05 at 16:45 -0700, Sean Christopherson wrote:
> Per Intel's SDM, unsupported ENCLS leafs result in a #GP, not a #UD.
> SGX1 is a special snowflake as the SGX1 flag is used by the CPU as a
> "soft" disable, e.g. if software disables machine check reporting, i.e.
> having SGX but not SGX1 is effectively "SGX completely unsupported" and
> and thus #UDs.

If I recall correctly, this is an erratum which can clear SGX1 in CPUID while
the SGX flag is still in CPUID?

But I am not sure whether this part is relevant to this patch?  Because SDM
already says ENCLS causes #UD if SGX1 isn't present.  This patch changes
"unsupported leaf" from causing #UD to causing #GP, which is also documented in
SDM.

> 
> Fixes: 9798adbc04cf ("KVM: VMX: Frame in ENCLS handler for SGX virtualization")
> Cc: Binbin Wu <binbin.wu@linux.intel.com>
> Cc: Kai Huang <kai.huang@intel.com>
> Signed-off-by: Sean Christopherson <seanjc@google.com>
> ---
>  arch/x86/kvm/vmx/sgx.c | 15 +++++++++------
>  1 file changed, 9 insertions(+), 6 deletions(-)
> 
> diff --git a/arch/x86/kvm/vmx/sgx.c b/arch/x86/kvm/vmx/sgx.c
> index f881f6ff6408..1c092ab89c33 100644
> --- a/arch/x86/kvm/vmx/sgx.c
> +++ b/arch/x86/kvm/vmx/sgx.c
> @@ -350,11 +350,12 @@ static int handle_encls_einit(struct kvm_vcpu *vcpu)
>  
>  static inline bool encls_leaf_enabled_in_guest(struct kvm_vcpu *vcpu, u32 leaf)
>  {
> -	if (!enable_sgx || !guest_cpuid_has(vcpu, X86_FEATURE_SGX))
> -		return false;
> -
> +	/*
> +	 * ENCLS #UDs if SGX1 isn't supported, i.e. this point will be reached

Why #UDs instead of #UD?  Is #UD a verb?

> +	 * if and only if the SGX1 leafs are enabled.
> +	 */

Is it better to move "ENCLS #UDs if SGX1 isn't supported" part to ...

>  	if (leaf >= ECREATE && leaf <= ETRACK)
> -		return guest_cpuid_has(vcpu, X86_FEATURE_SGX1);
> +		return true;
>  
>  	if (leaf >= EAUG && leaf <= EMODT)
>  		return guest_cpuid_has(vcpu, X86_FEATURE_SGX2);
> @@ -373,9 +374,11 @@ int handle_encls(struct kvm_vcpu *vcpu)
>  {
>  	u32 leaf = (u32)kvm_rax_read(vcpu);
>  
> -	if (!encls_leaf_enabled_in_guest(vcpu, leaf)) {
> +	if (!enable_sgx || !guest_cpuid_has(vcpu, X86_FEATURE_SGX) ||
> +	    !guest_cpuid_has(vcpu, X86_FEATURE_SGX1)) {
>  		kvm_queue_exception(vcpu, UD_VECTOR);

... above here, where the actual code reside?

> -	} else if (!sgx_enabled_in_guest_bios(vcpu) || !is_paging(vcpu)) {
> +	} else if (!encls_leaf_enabled_in_guest(vcpu, leaf) ||
> +		   !sgx_enabled_in_guest_bios(vcpu) || !is_paging(vcpu)) {
>  		kvm_inject_gp(vcpu, 0);
>  	} else {
>  		if (leaf == ECREATE)
> -- 
> 2.40.0.348.gf938b09366-goog
> 

Re: [PATCH 0/2] KVM: VMX: Fixes for faults on ENCLS emulation

[Huang, Kai]

On Wed, 2023-04-05 at 16:45 -0700, Sean Christopherson wrote:
> Found-by-inspection (when reviewing Binbin's patch) fixes for incorrect
> emulation of faults when KVMintercepts and emulates (sort of) ENCLS.
> 
> Very much compile tested only.  Ideally, someone with SGX hardware can
> confirm that these patches are correct, e.g. my assessment that KVM needs
> to manually check CR0.PG is based purely of SDM pseudocode.

Thanks for the patches.  I don't have a "ready" SGX environment at hand, but
I'll try to test or ask someone else to test after Easter holiday.

> 
> Sean Christopherson (2):
>   KVM: VMX: Inject #GP on ENCLS if vCPU has paging disabled (CR0.PG==0)
>   KVM: VMX: Inject #GP, not #UD, if SGX2 ENCLS leafs are unsupported
> 
>  arch/x86/kvm/vmx/sgx.c | 15 +++++++++------
>  1 file changed, 9 insertions(+), 6 deletions(-)
> 
> 
> base-commit: 27d6845d258b67f4eb3debe062b7dacc67e0c393
> -- 
> 2.40.0.348.gf938b09366-goog
> 

Re: [PATCH 2/2] KVM: VMX: Inject #GP, not #UD, if SGX2 ENCLS leafs are unsupported

[Sean Christopherson]

On Thu, Apr 06, 2023, Huang, Kai wrote:
> On Wed, 2023-04-05 at 16:45 -0700, Sean Christopherson wrote:
> > Per Intel's SDM, unsupported ENCLS leafs result in a #GP, not a #UD.
> > SGX1 is a special snowflake as the SGX1 flag is used by the CPU as a
> > "soft" disable, e.g. if software disables machine check reporting, i.e.
> > having SGX but not SGX1 is effectively "SGX completely unsupported" and
> > and thus #UDs.
> 
> If I recall correctly, this is an erratum which can clear SGX1 in CPUID while
> the SGX flag is still in CPUID?

Nope, not an erratum, architectural behavior.

> But I am not sure whether this part is relevant to this patch?  Because SDM
> already says ENCLS causes #UD if SGX1 isn't present.  This patch changes
> "unsupported leaf" from causing #UD to causing #GP, which is also documented in
> SDM.

I wanted to capture why SGX1 is different and given special treatment in the SDM.
I.e. to explain why SGX1 leafs are an exception to the "#GP if leaf unsupported"
clause.

> > Fixes: 9798adbc04cf ("KVM: VMX: Frame in ENCLS handler for SGX virtualization")
> > Cc: Binbin Wu <binbin.wu@linux.intel.com>
> > Cc: Kai Huang <kai.huang@intel.com>
> > Signed-off-by: Sean Christopherson <seanjc@google.com>
> > ---
> >  arch/x86/kvm/vmx/sgx.c | 15 +++++++++------
> >  1 file changed, 9 insertions(+), 6 deletions(-)
> > 
> > diff --git a/arch/x86/kvm/vmx/sgx.c b/arch/x86/kvm/vmx/sgx.c
> > index f881f6ff6408..1c092ab89c33 100644
> > --- a/arch/x86/kvm/vmx/sgx.c
> > +++ b/arch/x86/kvm/vmx/sgx.c
> > @@ -350,11 +350,12 @@ static int handle_encls_einit(struct kvm_vcpu *vcpu)
> >  
> >  static inline bool encls_leaf_enabled_in_guest(struct kvm_vcpu *vcpu, u32 leaf)
> >  {
> > -	if (!enable_sgx || !guest_cpuid_has(vcpu, X86_FEATURE_SGX))
> > -		return false;
> > -
> > +	/*
> > +	 * ENCLS #UDs if SGX1 isn't supported, i.e. this point will be reached
> 
> Why #UDs instead of #UD?  Is #UD a verb?

Heh, it is now ;-)  I can reword to something like

	/*
	 * ENCLS generates a #UD if SGX1 isn't supported ...
	 */

if my made up grammar is confusing.

> > +	 * if and only if the SGX1 leafs are enabled.
> > +	 */
> 
> Is it better to move "ENCLS #UDs if SGX1 isn't supported" part to ...
> 
> >  	if (leaf >= ECREATE && leaf <= ETRACK)
> > -		return guest_cpuid_has(vcpu, X86_FEATURE_SGX1);
> > +		return true;
> >  
> >  	if (leaf >= EAUG && leaf <= EMODT)
> >  		return guest_cpuid_has(vcpu, X86_FEATURE_SGX2);
> > @@ -373,9 +374,11 @@ int handle_encls(struct kvm_vcpu *vcpu)
> >  {
> >  	u32 leaf = (u32)kvm_rax_read(vcpu);
> >  
> > -	if (!encls_leaf_enabled_in_guest(vcpu, leaf)) {
> > +	if (!enable_sgx || !guest_cpuid_has(vcpu, X86_FEATURE_SGX) ||
> > +	    !guest_cpuid_has(vcpu, X86_FEATURE_SGX1)) {
> >  		kvm_queue_exception(vcpu, UD_VECTOR);
> 
> ... above here, where the actual code reside?

My goal was to document why encls_leaf_enabled_in_guest() unconditionally returns
true for SGX1 leafs, i.e. why it doesn't query X86_FEATURE_SGX1.  I'm definitely
not opposed to also adding a comment here, but I do want to keep the comment in
encls_leaf_enabled_in_guest().

Re: [PATCH 2/2] KVM: VMX: Inject #GP, not #UD, if SGX2 ENCLS leafs are unsupported

[Huang, Kai]

On Thu, 2023-04-06 at 11:00 -0700, Sean Christopherson wrote:
> On Thu, Apr 06, 2023, Huang, Kai wrote:
> > On Wed, 2023-04-05 at 16:45 -0700, Sean Christopherson wrote:
> > > Per Intel's SDM, unsupported ENCLS leafs result in a #GP, not a #UD.
> > > SGX1 is a special snowflake as the SGX1 flag is used by the CPU as a
> > > "soft" disable, e.g. if software disables machine check reporting, i.e.
> > > having SGX but not SGX1 is effectively "SGX completely unsupported" and
> > > and thus #UDs.
> > 
> > If I recall correctly, this is an erratum which can clear SGX1 in CPUID while
> > the SGX flag is still in CPUID?
> 
> Nope, not an erratum, architectural behavior.

I found the relevant section in SDM:

All supported IA32_MCi_CTL bits for all the machine check banks must be set for
Intel SGX to be available
(CPUID.SGX_Leaf.0:EAX[SGX1] == 1). Any act of clearing bits from '1 to '0 in any
of the IA32_MCi_CTL register
may disable Intel SGX (set CPUID.SGX_Leaf.0:EAX[SGX1] to 0) until the next
reset.

Looking at the code, it seems currently KVM won't clear SGX1 bit in CPUID when
guest disables IA32_MCi_CTL (writing 0).  Should we do that?

> 
> > But I am not sure whether this part is relevant to this patch?  Because SDM
> > already says ENCLS causes #UD if SGX1 isn't present.  This patch changes
> > "unsupported leaf" from causing #UD to causing #GP, which is also documented in
> > SDM.
> 
> I wanted to capture why SGX1 is different and given special treatment in the SDM.
> I.e. to explain why SGX1 leafs are an exception to the "#GP if leaf unsupported"
> clause.

OK.

> 
> > > Fixes: 9798adbc04cf ("KVM: VMX: Frame in ENCLS handler for SGX virtualization")
> > > Cc: Binbin Wu <binbin.wu@linux.intel.com>
> > > Cc: Kai Huang <kai.huang@intel.com>
> > > Signed-off-by: Sean Christopherson <seanjc@google.com>
> > > ---
> > >  arch/x86/kvm/vmx/sgx.c | 15 +++++++++------
> > >  1 file changed, 9 insertions(+), 6 deletions(-)
> > > 
> > > diff --git a/arch/x86/kvm/vmx/sgx.c b/arch/x86/kvm/vmx/sgx.c
> > > index f881f6ff6408..1c092ab89c33 100644
> > > --- a/arch/x86/kvm/vmx/sgx.c
> > > +++ b/arch/x86/kvm/vmx/sgx.c
> > > @@ -350,11 +350,12 @@ static int handle_encls_einit(struct kvm_vcpu *vcpu)
> > >  
> > >  static inline bool encls_leaf_enabled_in_guest(struct kvm_vcpu *vcpu, u32 leaf)
> > >  {
> > > -	if (!enable_sgx || !guest_cpuid_has(vcpu, X86_FEATURE_SGX))
> > > -		return false;
> > > -
> > > +	/*
> > > +	 * ENCLS #UDs if SGX1 isn't supported, i.e. this point will be reached
> > 
> > Why #UDs instead of #UD?  Is #UD a verb?
> 
> Heh, it is now ;-)  I can reword to something like
> 
> 	/*
> 	 * ENCLS generates a #UD if SGX1 isn't supported ...
> 	 */
> 
> if my made up grammar is confusing.
> 
> > > +	 * if and only if the SGX1 leafs are enabled.
> > > +	 */
> > 
> > Is it better to move "ENCLS #UDs if SGX1 isn't supported" part to ...
> > 
> > >  	if (leaf >= ECREATE && leaf <= ETRACK)
> > > -		return guest_cpuid_has(vcpu, X86_FEATURE_SGX1);
> > > +		return true;
> > >  
> > >  	if (leaf >= EAUG && leaf <= EMODT)
> > >  		return guest_cpuid_has(vcpu, X86_FEATURE_SGX2);
> > > @@ -373,9 +374,11 @@ int handle_encls(struct kvm_vcpu *vcpu)
> > >  {
> > >  	u32 leaf = (u32)kvm_rax_read(vcpu);
> > >  
> > > -	if (!encls_leaf_enabled_in_guest(vcpu, leaf)) {
> > > +	if (!enable_sgx || !guest_cpuid_has(vcpu, X86_FEATURE_SGX) ||
> > > +	    !guest_cpuid_has(vcpu, X86_FEATURE_SGX1)) {
> > >  		kvm_queue_exception(vcpu, UD_VECTOR);
> > 
> > ... above here, where the actual code reside?
> 
> My goal was to document why encls_leaf_enabled_in_guest() unconditionally returns
> true for SGX1 leafs, i.e. why it doesn't query X86_FEATURE_SGX1.  I'm definitely
> not opposed to also adding a comment here, but I do want to keep the comment in
> encls_leaf_enabled_in_guest().

Sure.

Anyway,

Reviewed-by: Kai Huang <kai.huang@intel.com>

Re: [PATCH 2/2] KVM: VMX: Inject #GP, not #UD, if SGX2 ENCLS leafs are unsupported

[Sean Christopherson]

On Wed, Apr 12, 2023, Kai Huang wrote:
> On Thu, 2023-04-06 at 11:00 -0700, Sean Christopherson wrote:
> > On Thu, Apr 06, 2023, Huang, Kai wrote:
> > > On Wed, 2023-04-05 at 16:45 -0700, Sean Christopherson wrote:
> > > > Per Intel's SDM, unsupported ENCLS leafs result in a #GP, not a #UD.
> > > > SGX1 is a special snowflake as the SGX1 flag is used by the CPU as a
> > > > "soft" disable, e.g. if software disables machine check reporting, i.e.
> > > > having SGX but not SGX1 is effectively "SGX completely unsupported" and
> > > > and thus #UDs.
> > > 
> > > If I recall correctly, this is an erratum which can clear SGX1 in CPUID while
> > > the SGX flag is still in CPUID?
> > 
> > Nope, not an erratum, architectural behavior.
> 
> I found the relevant section in SDM:
> 
> All supported IA32_MCi_CTL bits for all the machine check banks must be set
> for Intel SGX to be available (CPUID.SGX_Leaf.0:EAX[SGX1] == 1). Any act of
> clearing bits from '1 to '0 in any of the IA32_MCi_CTL register may disable
> Intel SGX (set CPUID.SGX_Leaf.0:EAX[SGX1] to 0) until the next reset.
> 
> Looking at the code, it seems currently KVM won't clear SGX1 bit in CPUID when
> guest disables IA32_MCi_CTL (writing 0).  Should we do that?

No, the behavior isn't strictly required: clearing bits *may* disable Intel SGX.
And there is zero benefit to the guest, the behavior exists in bare metal purely
to allow the CPU to provide security guarantees.  On the flip side, emulating the
disabling of SGX without causing problems, e.g. when userspace sets MSRs, would be
quite tricky.

Re: [PATCH 2/2] KVM: VMX: Inject #GP, not #UD, if SGX2 ENCLS leafs are unsupported

[Huang, Kai]

On Wed, 2023-04-12 at 07:38 -0700, Sean Christopherson wrote:
> On Wed, Apr 12, 2023, Kai Huang wrote:
> > On Thu, 2023-04-06 at 11:00 -0700, Sean Christopherson wrote:
> > > On Thu, Apr 06, 2023, Huang, Kai wrote:
> > > > On Wed, 2023-04-05 at 16:45 -0700, Sean Christopherson wrote:
> > > > > Per Intel's SDM, unsupported ENCLS leafs result in a #GP, not a #UD.
> > > > > SGX1 is a special snowflake as the SGX1 flag is used by the CPU as a
> > > > > "soft" disable, e.g. if software disables machine check reporting, i.e.
> > > > > having SGX but not SGX1 is effectively "SGX completely unsupported" and
> > > > > and thus #UDs.
> > > > 
> > > > If I recall correctly, this is an erratum which can clear SGX1 in CPUID while
> > > > the SGX flag is still in CPUID?
> > > 
> > > Nope, not an erratum, architectural behavior.
> > 
> > I found the relevant section in SDM:
> > 
> > All supported IA32_MCi_CTL bits for all the machine check banks must be set
> > for Intel SGX to be available (CPUID.SGX_Leaf.0:EAX[SGX1] == 1). Any act of
> > clearing bits from '1 to '0 in any of the IA32_MCi_CTL register may disable
> > Intel SGX (set CPUID.SGX_Leaf.0:EAX[SGX1] to 0) until the next reset.
> > 
> > Looking at the code, it seems currently KVM won't clear SGX1 bit in CPUID when
> > guest disables IA32_MCi_CTL (writing 0).  Should we do that?
> 
> No, the behavior isn't strictly required: clearing bits *may* disable Intel SGX.
> And there is zero benefit to the guest, the behavior exists in bare metal purely
> to allow the CPU to provide security guarantees.  On the flip side, emulating the
> disabling of SGX without causing problems, e.g. when userspace sets MSRs, would be
> quite tricky.

Yeah my thinking too.  Agreed.

Re: [PATCH 2/2] KVM: VMX: Inject #GP, not #UD, if SGX2 ENCLS leafs are unsupported

[Huang, Kai]

On Wed, 2023-04-12 at 11:15 +0000, Huang, Kai wrote:
> On Thu, 2023-04-06 at 11:00 -0700, Sean Christopherson wrote:
> > On Thu, Apr 06, 2023, Huang, Kai wrote:
> > > On Wed, 2023-04-05 at 16:45 -0700, Sean Christopherson wrote:
> > > > Per Intel's SDM, unsupported ENCLS leafs result in a #GP, not a #UD.
> > > > SGX1 is a special snowflake as the SGX1 flag is used by the CPU as a
> > > > "soft" disable, e.g. if software disables machine check reporting, i.e.
> > > > having SGX but not SGX1 is effectively "SGX completely unsupported" and
> > > > and thus #UDs.
> > > 

[...]

> 
> Reviewed-by: Kai Huang <kai.huang@intel.com>
> 
> 

Tested by disabling SGX2 in the guest and running SGX2 leaf inside, and in my
testing the #GP is injected to the guest due to leaf not enabled.

Tested-by: Kai Huang <kai.huang@intel.com>

Re: [PATCH 1/2] KVM: VMX: Inject #GP on ENCLS if vCPU has paging disabled (CR0.PG==0)

[Huang, Kai]

On Thu, 2023-04-06 at 08:54 +0000, Huang, Kai wrote:
> On Wed, 2023-04-05 at 16:45 -0700, Sean Christopherson wrote:
> > Inject a #GP when emulating/forwarding a valid ENCLS leaf if the vCPU has
> > paging disabled, e.g. if KVM is intercepting ECREATE to enforce additional
> > restrictions.  The pseudocode in the SDM lists all #GP triggers, including
> > CR0.PG=0, as being checked after the ENLCS-exiting checks, i.e. the
> > VM-Exit will occur before the CPU performs the CR0.PG check.
> > 
> > Fixes: 70210c044b4e ("KVM: VMX: Add SGX ENCLS[ECREATE] handler to enforce CPUID restrictions")
> > Cc: Binbin Wu <binbin.wu@linux.intel.com>
> > Cc: Kai Huang <kai.huang@intel.com>
> > Signed-off-by: Sean Christopherson <seanjc@google.com>
> 
> Reviewed-by: Kai Huang <kai.huang@intel.com>
> 
> > 

Tested by running ENCLS in protected mode before enabling paging, and in my test
the #GP was injected to the guest.

Tested-by: Kai Huang <kai.huang@intel.com>

Re: [PATCH 0/2] KVM: VMX: Fixes for faults on ENCLS emulation

[Sean Christopherson]

On Wed, 05 Apr 2023 16:45:54 -0700, Sean Christopherson wrote:
> Found-by-inspection (when reviewing Binbin's patch) fixes for incorrect
> emulation of faults when KVMintercepts and emulates (sort of) ENCLS.
> 
> Very much compile tested only.  Ideally, someone with SGX hardware can
> confirm that these patches are correct, e.g. my assessment that KVM needs
> to manually check CR0.PG is based purely of SDM pseudocode.
> 
> [...]

Applied to kvm-x86 vmx, thanks!

[1/2] KVM: VMX: Inject #GP on ENCLS if vCPU has paging disabled (CR0.PG==0)
      https://github.com/kvm-x86/linux/commit/5e50082c8c21
[2/2] KVM: VMX: Inject #GP, not #UD, if SGX2 ENCLS leafs are unsupported
      https://github.com/kvm-x86/linux/commit/c3a1e119a343

--
https://github.com/kvm-x86/linux/tree/next
https://github.com/kvm-x86/linux/tree/fixes