diff mbox series

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

Message ID 20230405234556.696927-3-seanjc@google.com
State New
Headers
Series KVM: VMX: Fixes for faults on ENCLS emulation |

Commit Message

Sean Christopherson April 5, 2023, 11:45 p.m. UTC 
  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(-)

Comments

Kai Huang April 6, 2023, 9:17 a.m. UTC | #1 
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
>
Sean Christopherson April 6, 2023, 6 p.m. UTC | #2 
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().
Kai Huang April 12, 2023, 11:15 a.m. UTC | #3 
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>
Sean Christopherson April 12, 2023, 2:38 p.m. UTC | #4 
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.
Kai Huang April 12, 2023, 10:28 p.m. UTC | #5 
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.
Kai Huang April 21, 2023, 9:17 a.m. UTC | #6 
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>
diff mbox series

Patch

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)