KVM Archive on lore.kernel.org
 help / color / Atom feed
* [PATCH] KVM: x86: Perform non-canonical checks in 32-bit KVM
@ 2020-01-15 18:36 Sean Christopherson
  2020-01-16  1:37 ` Krish Sadhukhan
  0 siblings, 1 reply; 4+ messages in thread
From: Sean Christopherson @ 2020-01-15 18:36 UTC (permalink / raw)
  To: Paolo Bonzini
  Cc: Sean Christopherson, Vitaly Kuznetsov, Wanpeng Li, Jim Mattson,
	Joerg Roedel, kvm, linux-kernel

Remove the CONFIG_X86_64 condition from the low level non-canonical
helpers to effectively enable non-canonical checks on 32-bit KVM.
Non-canonical checks are performed by hardware if the CPU *supports*
64-bit mode, whether or not the CPU is actually in 64-bit mode is
irrelevant.

For the most part, skipping non-canonical checks on 32-bit KVM is ok-ish
because 32-bit KVM always (hopefully) drops bits 63:32 of whatever value
it's checking before propagating it to hardware, and architecturally,
the expected behavior for the guest is a bit of a grey area since the
vCPU itself doesn't support 64-bit mode.  I.e. a 32-bit KVM guest can
observe the missed checks in several paths, e.g. INVVPID and VM-Enter,
but it's debatable whether or not the missed checks constitute a bug
because technically the vCPU doesn't support 64-bit mode.

The primary motivation for enabling the non-canonical checks is defense
in depth.  As mentioned above, a guest can trigger a missed check via
INVVPID or VM-Enter.  INVVPID is straightforward as it takes a 64-bit
virtual address as part of its 128-bit INVVPID descriptor and fails if
the address is non-canonical, even if INVVPID is executed in 32-bit PM.
Nested VM-Enter is a bit more convoluted as it requires the guest to
write natural width VMCS fields via memory accesses and then VMPTRLD the
VMCS, but it's still possible.  In both cases, KVM is saved from a true
bug only because its flows that propagate values to hardware (correctly)
take "unsigned long" parameters and so drop bits 63:32 of the bad value.

Explicitly performing the non-canonical checks makes it less likely that
a bad value will be propagated to hardware, e.g. in the INVVPID case,
if __invvpid() didn't implicitly drop bits 63:32 then KVM would BUG() on
the resulting unexpected INVVPID failure due to hardware rejecting the
non-canonical address.

The only downside to enabling the non-canonical checks is that it adds a
relatively small amount of overhead, but the affected flows are not hot
paths, i.e. the overhead is negligible.

Note, KVM technically could gate the non-canonical checks on 32-bit KVM
with static_cpu_has(X86_FEATURE_LM), but on bare metal that's an even
bigger waste of code for everyone except the 0.00000000000001% of the
population running on Yonah, and nested 32-bit on 64-bit already fudges
things with respect to 64-bit CPU behavior.

Signed-off-by: Sean Christopherson <sean.j.christopherson@intel.com>
---
 arch/x86/kvm/x86.h | 8 --------
 1 file changed, 8 deletions(-)

diff --git a/arch/x86/kvm/x86.h b/arch/x86/kvm/x86.h
index cab5e71f0f0f..3ff590ec0238 100644
--- a/arch/x86/kvm/x86.h
+++ b/arch/x86/kvm/x86.h
@@ -166,21 +166,13 @@ static inline u64 get_canonical(u64 la, u8 vaddr_bits)
 
 static inline bool is_noncanonical_address(u64 la, struct kvm_vcpu *vcpu)
 {
-#ifdef CONFIG_X86_64
 	return get_canonical(la, vcpu_virt_addr_bits(vcpu)) != la;
-#else
-	return false;
-#endif
 }
 
 static inline bool emul_is_noncanonical_address(u64 la,
 						struct x86_emulate_ctxt *ctxt)
 {
-#ifdef CONFIG_X86_64
 	return get_canonical(la, ctxt_virt_addr_bits(ctxt)) != la;
-#else
-	return false;
-#endif
 }
 
 static inline void vcpu_cache_mmio_info(struct kvm_vcpu *vcpu,
-- 
2.24.1


^ permalink raw reply	[flat|nested] 4+ messages in thread

* Re: [PATCH] KVM: x86: Perform non-canonical checks in 32-bit KVM
  2020-01-15 18:36 [PATCH] KVM: x86: Perform non-canonical checks in 32-bit KVM Sean Christopherson
@ 2020-01-16  1:37 ` Krish Sadhukhan
  2020-01-16 15:50   ` Sean Christopherson
  0 siblings, 1 reply; 4+ messages in thread
From: Krish Sadhukhan @ 2020-01-16  1:37 UTC (permalink / raw)
  To: Sean Christopherson, Paolo Bonzini
  Cc: Vitaly Kuznetsov, Wanpeng Li, Jim Mattson, Joerg Roedel, kvm,
	linux-kernel



On 01/15/2020 10:36 AM, Sean Christopherson wrote:
> Remove the CONFIG_X86_64 condition from the low level non-canonical
> helpers to effectively enable non-canonical checks on 32-bit KVM.
> Non-canonical checks are performed by hardware if the CPU *supports*
> 64-bit mode, whether or not the CPU is actually in 64-bit mode is
> irrelevant.
>
> For the most part, skipping non-canonical checks on 32-bit KVM is ok-ish
> because 32-bit KVM always (hopefully) drops bits 63:32 of whatever value
> it's checking before propagating it to hardware, and architecturally,
> the expected behavior for the guest is a bit of a grey area since the
> vCPU itself doesn't support 64-bit mode.  I.e. a 32-bit KVM guest can
> observe the missed checks in several paths, e.g. INVVPID and VM-Enter,
> but it's debatable whether or not the missed checks constitute a bug
> because technically the vCPU doesn't support 64-bit mode.
>
> The primary motivation for enabling the non-canonical checks is defense
> in depth.  As mentioned above, a guest can trigger a missed check via
> INVVPID or VM-Enter.  INVVPID is straightforward as it takes a 64-bit
> virtual address as part of its 128-bit INVVPID descriptor and fails if
> the address is non-canonical, even if INVVPID is executed in 32-bit PM.
> Nested VM-Enter is a bit more convoluted as it requires the guest to
> write natural width VMCS fields via memory accesses and then VMPTRLD the
> VMCS, but it's still possible.  In both cases, KVM is saved from a true
> bug only because its flows that propagate values to hardware (correctly)
> take "unsigned long" parameters and so drop bits 63:32 of the bad value.
>
> Explicitly performing the non-canonical checks makes it less likely that
> a bad value will be propagated to hardware, e.g. in the INVVPID case,
> if __invvpid() didn't implicitly drop bits 63:32 then KVM would BUG() on
> the resulting unexpected INVVPID failure due to hardware rejecting the
> non-canonical address.
>
> The only downside to enabling the non-canonical checks is that it adds a
> relatively small amount of overhead, but the affected flows are not hot
> paths, i.e. the overhead is negligible.
>
> Note, KVM technically could gate the non-canonical checks on 32-bit KVM
> with static_cpu_has(X86_FEATURE_LM), but on bare metal that's an even
> bigger waste of code for everyone except the 0.00000000000001% of the
> population running on Yonah, and nested 32-bit on 64-bit already fudges
> things with respect to 64-bit CPU behavior.
>
> Signed-off-by: Sean Christopherson <sean.j.christopherson@intel.com>
> ---
>   arch/x86/kvm/x86.h | 8 --------
>   1 file changed, 8 deletions(-)
>
> diff --git a/arch/x86/kvm/x86.h b/arch/x86/kvm/x86.h
> index cab5e71f0f0f..3ff590ec0238 100644
> --- a/arch/x86/kvm/x86.h
> +++ b/arch/x86/kvm/x86.h
> @@ -166,21 +166,13 @@ static inline u64 get_canonical(u64 la, u8 vaddr_bits)
>   
>   static inline bool is_noncanonical_address(u64 la, struct kvm_vcpu *vcpu)
>   {
> -#ifdef CONFIG_X86_64
>   	return get_canonical(la, vcpu_virt_addr_bits(vcpu)) != la;
> -#else
> -	return false;
> -#endif
>   }
>   
>   static inline bool emul_is_noncanonical_address(u64 la,
>   						struct x86_emulate_ctxt *ctxt)
>   {
> -#ifdef CONFIG_X86_64
>   	return get_canonical(la, ctxt_virt_addr_bits(ctxt)) != la;
> -#else
> -	return false;
> -#endif
>   }
>   
>   static inline void vcpu_cache_mmio_info(struct kvm_vcpu *vcpu,

nested_vmx_check_host_state() still won't call it on 32-bit because it 
has the CONFIG_X86_64 guard around the callee:

  #ifdef CONFIG_X86_64
         if (CC(is_noncanonical_address(vmcs12->host_fs_base, vcpu)) ||
             CC(is_noncanonical_address(vmcs12->host_gs_base, vcpu)) ||
  ...


Don't we need to remove these guards in the callers as well ?

^ permalink raw reply	[flat|nested] 4+ messages in thread

* Re: [PATCH] KVM: x86: Perform non-canonical checks in 32-bit KVM
  2020-01-16  1:37 ` Krish Sadhukhan
@ 2020-01-16 15:50   ` Sean Christopherson
  2020-01-18 21:38     ` Paolo Bonzini
  0 siblings, 1 reply; 4+ messages in thread
From: Sean Christopherson @ 2020-01-16 15:50 UTC (permalink / raw)
  To: Krish Sadhukhan
  Cc: Paolo Bonzini, Vitaly Kuznetsov, Wanpeng Li, Jim Mattson,
	Joerg Roedel, kvm, linux-kernel

On Wed, Jan 15, 2020 at 05:37:16PM -0800, Krish Sadhukhan wrote:
> 
> On 01/15/2020 10:36 AM, Sean Christopherson wrote:
> >  arch/x86/kvm/x86.h | 8 --------
> >  1 file changed, 8 deletions(-)
> >
> >diff --git a/arch/x86/kvm/x86.h b/arch/x86/kvm/x86.h
> >index cab5e71f0f0f..3ff590ec0238 100644
> >--- a/arch/x86/kvm/x86.h
> >+++ b/arch/x86/kvm/x86.h
> >@@ -166,21 +166,13 @@ static inline u64 get_canonical(u64 la, u8 vaddr_bits)
> >  static inline bool is_noncanonical_address(u64 la, struct kvm_vcpu *vcpu)
> >  {
> >-#ifdef CONFIG_X86_64
> >  	return get_canonical(la, vcpu_virt_addr_bits(vcpu)) != la;
> >-#else
> >-	return false;
> >-#endif
> >  }
> >  static inline bool emul_is_noncanonical_address(u64 la,
> >  						struct x86_emulate_ctxt *ctxt)
> >  {
> >-#ifdef CONFIG_X86_64
> >  	return get_canonical(la, ctxt_virt_addr_bits(ctxt)) != la;
> >-#else
> >-	return false;
> >-#endif
> >  }
> >  static inline void vcpu_cache_mmio_info(struct kvm_vcpu *vcpu,
> 
> nested_vmx_check_host_state() still won't call it on 32-bit because it has
> the CONFIG_X86_64 guard around the callee:
> 
>  #ifdef CONFIG_X86_64
>         if (CC(is_noncanonical_address(vmcs12->host_fs_base, vcpu)) ||
>             CC(is_noncanonical_address(vmcs12->host_gs_base, vcpu)) ||
>  ...

Doh, I was looking at an older version of nested.c.  Nice catch!

> Don't we need to remove these guards in the callers as well ?

Ya, that would be my preference.

^ permalink raw reply	[flat|nested] 4+ messages in thread

* Re: [PATCH] KVM: x86: Perform non-canonical checks in 32-bit KVM
  2020-01-16 15:50   ` Sean Christopherson
@ 2020-01-18 21:38     ` Paolo Bonzini
  0 siblings, 0 replies; 4+ messages in thread
From: Paolo Bonzini @ 2020-01-18 21:38 UTC (permalink / raw)
  To: Sean Christopherson, Krish Sadhukhan
  Cc: Vitaly Kuznetsov, Wanpeng Li, Jim Mattson, Joerg Roedel, kvm,
	linux-kernel

On 16/01/20 16:50, Sean Christopherson wrote:
> On Wed, Jan 15, 2020 at 05:37:16PM -0800, Krish Sadhukhan wrote:
>>
>> On 01/15/2020 10:36 AM, Sean Christopherson wrote:
>>>  arch/x86/kvm/x86.h | 8 --------
>>>  1 file changed, 8 deletions(-)
>>>
>>> diff --git a/arch/x86/kvm/x86.h b/arch/x86/kvm/x86.h
>>> index cab5e71f0f0f..3ff590ec0238 100644
>>> --- a/arch/x86/kvm/x86.h
>>> +++ b/arch/x86/kvm/x86.h
>>> @@ -166,21 +166,13 @@ static inline u64 get_canonical(u64 la, u8 vaddr_bits)
>>>  static inline bool is_noncanonical_address(u64 la, struct kvm_vcpu *vcpu)
>>>  {
>>> -#ifdef CONFIG_X86_64
>>>  	return get_canonical(la, vcpu_virt_addr_bits(vcpu)) != la;
>>> -#else
>>> -	return false;
>>> -#endif
>>>  }
>>>  static inline bool emul_is_noncanonical_address(u64 la,
>>>  						struct x86_emulate_ctxt *ctxt)
>>>  {
>>> -#ifdef CONFIG_X86_64
>>>  	return get_canonical(la, ctxt_virt_addr_bits(ctxt)) != la;
>>> -#else
>>> -	return false;
>>> -#endif
>>>  }
>>>  static inline void vcpu_cache_mmio_info(struct kvm_vcpu *vcpu,
>>
>> nested_vmx_check_host_state() still won't call it on 32-bit because it has
>> the CONFIG_X86_64 guard around the callee:
>>
>>  #ifdef CONFIG_X86_64
>>         if (CC(is_noncanonical_address(vmcs12->host_fs_base, vcpu)) ||
>>             CC(is_noncanonical_address(vmcs12->host_gs_base, vcpu)) ||
>>  ...
> 
> Doh, I was looking at an older version of nested.c.  Nice catch!
> 
>> Don't we need to remove these guards in the callers as well ?
> 
> Ya, that would be my preference.
> 

Fixed and queued, thanks.

Paolo


^ permalink raw reply	[flat|nested] 4+ messages in thread

end of thread, back to index

Thread overview: 4+ messages (download: mbox.gz / follow: Atom feed)
-- links below jump to the message on this page --
2020-01-15 18:36 [PATCH] KVM: x86: Perform non-canonical checks in 32-bit KVM Sean Christopherson
2020-01-16  1:37 ` Krish Sadhukhan
2020-01-16 15:50   ` Sean Christopherson
2020-01-18 21:38     ` Paolo Bonzini

KVM Archive on lore.kernel.org

Archives are clonable:
	git clone --mirror https://lore.kernel.org/kvm/0 kvm/git/0.git

	# If you have public-inbox 1.1+ installed, you may
	# initialize and index your mirror using the following commands:
	public-inbox-init -V2 kvm kvm/ https://lore.kernel.org/kvm \
		kvm@vger.kernel.org
	public-inbox-index kvm

Example config snippet for mirrors

Newsgroup available over NNTP:
	nntp://nntp.lore.kernel.org/org.kernel.vger.kvm


AGPL code for this site: git clone https://public-inbox.org/public-inbox.git