Messages in this thread | | | Subject | Re: [PATCH] KVM: x86: Perform non-canonical checks in 32-bit KVM | From | Krish Sadhukhan <> | Date | Wed, 15 Jan 2020 17:37:16 -0800 |
| |
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 ?
| |