[<prev] [next>] [<thread-prev] [thread-next>] [day] [month] [year] [list]
Message-ID: <ZyLTRvbLq9ZTXBim@google.com>
Date: Wed, 30 Oct 2024 17:45:58 -0700
From: Sean Christopherson <seanjc@...gle.com>
To: Maxim Levitsky <mlevitsk@...hat.com>
Cc: kvm@...r.kernel.org, Thomas Gleixner <tglx@...utronix.de>,
Paolo Bonzini <pbonzini@...hat.com>, Ingo Molnar <mingo@...hat.com>,
Vitaly Kuznetsov <vkuznets@...hat.com>, linux-kernel@...r.kernel.org,
"H. Peter Anvin" <hpa@...or.com>, x86@...nel.org, Borislav Petkov <bp@...en8.de>,
Dave Hansen <dave.hansen@...ux.intel.com>
Subject: Re: [PATCH v4 3/4] KVM: x86: model canonical checks more precisely
On Fri, Sep 06, 2024, Maxim Levitsky wrote:
> As a result of a recent investigation, it was determined that x86 CPUs
> which support 5-level paging, don't always respect CR4.LA57 when doing
> canonical checks.
>
> In particular:
>
> 1. MSRs which contain a linear address, allow full 57-bitcanonical address
> regardless of CR4.LA57 state. For example: MSR_KERNEL_GS_BASE.
>
> 2. All hidden segment bases and GDT/IDT bases also behave like MSRs.
> This means that full 57-bit canonical address can be loaded to them
> regardless of CR4.LA57, both using MSRS (e.g GS_BASE) and instructions
> (e.g LGDT).
>
> 3. TLB invalidation instructions also allow the user to use full 57-bit
> address regardless of the CR4.LA57.
>
> Finally, it must be noted that the CPU doesn't prevent the user from
> disabling 5-level paging, even when the full 57-bit canonical address is
> present in one of the registers mentioned above (e.g GDT base).
>
> In fact, this can happen without any userspace help, when the CPU enters
> SMM mode - some MSRs, for example MSR_KERNEL_GS_BASE are left to contain
> a non-canonical address in regard to the new mode.
>
> Since most of the affected MSRs and all segment bases can be read and
> written freely by the guest without any KVM intervention, this patch makes
> the emulator closely follow hardware behavior, which means that the
> emulator doesn't take in the account the guest CPUID support for 5-level
> paging, and only takes in the account the host CPU support.
>
> Signed-off-by: Maxim Levitsky <mlevitsk@...hat.com>
> ---
> arch/x86/kvm/emulate.c | 2 +-
> arch/x86/kvm/mmu/mmu.c | 2 +-
> arch/x86/kvm/vmx/nested.c | 22 ++++++++--------
> arch/x86/kvm/vmx/pmu_intel.c | 2 +-
> arch/x86/kvm/vmx/sgx.c | 2 +-
> arch/x86/kvm/vmx/vmx.c | 4 +--
> arch/x86/kvm/x86.c | 8 +++---
> arch/x86/kvm/x86.h | 49 ++++++++++++++++++++++++++++++++++--
> 8 files changed, 68 insertions(+), 23 deletions(-)
>
> diff --git a/arch/x86/kvm/emulate.c b/arch/x86/kvm/emulate.c
> index 8c8061884a019..60986f67c35a8 100644
> --- a/arch/x86/kvm/emulate.c
> +++ b/arch/x86/kvm/emulate.c
> @@ -654,7 +654,7 @@ static inline bool emul_is_noncanonical_address(u64 la,
> struct x86_emulate_ctxt *ctxt,
> unsigned int flags)
> {
> - return !ctxt->ops->is_canonical_addr(ctxt, la, 0);
> + return !ctxt->ops->is_canonical_addr(ctxt, la, flags);
And conversely, passing flags to ->is_canonical_addr() belongs in the patch that
adds the plumbing. Even though flags isn't truly consumed until this patch,
passing '0' in this helper is confusing and an unnecessary risk.
Powered by blists - more mailing lists