linux-kernel - Re: [PATCH 4/4] KVM: x86: Simplify logic to handle lack of host NX support

lists.openwall.net		lists / announce owl-users owl-dev john-users john-dev passwdqc-users yescrypt popa3d-users / oss-security kernel-hardening musl sabotage tlsify passwords / crypt-dev xvendor / Bugtraq Full-Disclosure linux-kernel linux-netdev linux-ext4 linux-hardening linux-cve-announce PHC
Open Source and information security mailing list archives

Hash Suite: Windows password security audit tool. GUI, reports in PDF.

[<prev] [next>] [<thread-prev] [thread-next>] [day] [month] [year] [list]

Message-ID: <CALMp9eRGj_5+dZXQazVEkeKeDnc7GFm1Vnt2RS_V6akAR=rZsA@mail.gmail.com>
Date:   Tue, 15 Jun 2021 15:58:24 -0700
From:   Jim Mattson <jmattson@...gle.com>
To:     Sean Christopherson <seanjc@...gle.com>
Cc:     Paolo Bonzini <pbonzini@...hat.com>,
        Vitaly Kuznetsov <vkuznets@...hat.com>,
        Wanpeng Li <wanpengli@...cent.com>,
        Joerg Roedel <joro@...tes.org>, kvm list <kvm@...r.kernel.org>,
        LKML <linux-kernel@...r.kernel.org>
Subject: Re: [PATCH 4/4] KVM: x86: Simplify logic to handle lack of host NX support

On Tue, Jun 15, 2021 at 9:45 AM Sean Christopherson <seanjc@...gle.com> wrote:
>
> Use boot_cpu_has() to check for NX support now that KVM requires
> host_efer.NX=1 if NX is supported.  Opportunistically avoid the guest
> CPUID lookup in cpuid_fix_nx_cap() if NX is supported, which is by far
> the common case.
>
> Signed-off-by: Sean Christopherson <seanjc@...gle.com>
> ---
>  arch/x86/kvm/cpuid.c | 13 +++++--------
>  1 file changed, 5 insertions(+), 8 deletions(-)
>
> diff --git a/arch/x86/kvm/cpuid.c b/arch/x86/kvm/cpuid.c
> index b4da665bb892..786f556302cd 100644
> --- a/arch/x86/kvm/cpuid.c
> +++ b/arch/x86/kvm/cpuid.c
> @@ -208,16 +208,14 @@ static void kvm_vcpu_after_set_cpuid(struct kvm_vcpu *vcpu)
>         kvm_mmu_reset_context(vcpu);
>  }
>
> -static int is_efer_nx(void)
> -{
> -       return host_efer & EFER_NX;
> -}
> -
>  static void cpuid_fix_nx_cap(struct kvm_vcpu *vcpu)
>  {
>         int i;
>         struct kvm_cpuid_entry2 *e, *entry;
>
> +       if (boot_cpu_has(X86_FEATURE_NX))
> +               return;
> +
>         entry = NULL;
>         for (i = 0; i < vcpu->arch.cpuid_nent; ++i) {
>                 e = &vcpu->arch.cpuid_entries[i];
> @@ -226,7 +224,7 @@ static void cpuid_fix_nx_cap(struct kvm_vcpu *vcpu)
>                         break;
>                 }
>         }
> -       if (entry && cpuid_entry_has(entry, X86_FEATURE_NX) && !is_efer_nx()) {
> +       if (entry && cpuid_entry_has(entry, X86_FEATURE_NX)) {
>                 cpuid_entry_clear(entry, X86_FEATURE_NX);
>                 printk(KERN_INFO "kvm: guest NX capability removed\n");
>         }

It would be nice if we chose one consistent approach to dealing with
invalid guest CPUID information and stuck with it. Silently modifying
the table provided by userspace seems wrong to me. I much prefer the
kvm_check_cpuid approach of telling userspace that the guest CPUID
information is invalid. (Of course, once we return -EINVAL for more
than one field, good luck figuring out which field is invalid!)

Reviewed-by: Jim Mattson <jmattson@...gle.com>