Re: [PATCH] KVM: x86: Perform non-canonical checks in 32-bit KVM

[Krish Sadhukhan <krish.sadhukhan@oracle.com>]

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 ?