| Message ID | 20200115183605.15413-1-sean.j.christopherson@intel.com (mailing list archive) |
|---|---|
| State | New, archived |
| Headers | show |
| Series | KVM: x86: Perform non-canonical checks in 32-bit KVM | expand |
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 ?
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.
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
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,
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(-)