Re: [PATCH] KVM: x86: add hint to skip hidden rdpkru under kvm_load_host_xsave_state

[Andy Lutomirski <luto@xxxxxxxxxx>]

On Fri, May 7, 2021 at 9:45 AM Jon Kohler <jon@xxxxxxxxxxx> wrote:
>
> kvm_load_host_xsave_state handles xsave on vm exit, part of which is
> managing memory protection key state. The latest arch.pkru is updated
> with a rdpkru, and if that doesn't match the base host_pkru (which
> about 70% of the time), we issue a __write_pkru.

This thread caused me to read the code, and I don't get how it's even
remotely correct.

First, kvm_load_guest_fpu() has this delight:

    /*
     * Guests with protected state can't have it set by the hypervisor,
     * so skip trying to set it.
     */
    if (vcpu->arch.guest_fpu)
        /* PKRU is separately restored in kvm_x86_ops.run. */
        __copy_kernel_to_fpregs(&vcpu->arch.guest_fpu->state,
                    ~XFEATURE_MASK_PKRU);

That's nice, but it fails to restore XINUSE[PKRU].  As far as I know,
that bit is live, and the only way to restore it to 0 is with
XRSTOR(S).

> diff --git a/arch/x86/kvm/x86.c b/arch/x86/kvm/x86.c
> index cebdaa1e3cf5..cd95adbd140c 100644
> --- a/arch/x86/kvm/x86.c
> +++ b/arch/x86/kvm/x86.c
> @@ -912,10 +912,10 @@ void kvm_load_guest_xsave_state(struct kvm_vcpu *vcpu)
>         }
>
>         if (static_cpu_has(X86_FEATURE_PKU) &&
> -           (kvm_read_cr4_bits(vcpu, X86_CR4_PKE) ||
> -            (vcpu->arch.xcr0 & XFEATURE_MASK_PKRU)) &&
> -           vcpu->arch.pkru != vcpu->arch.host_pkru)
> -               __write_pkru(vcpu->arch.pkru);
> +           vcpu->arch.pkru != vcpu->arch.host_pkru &&
> +           ((vcpu->arch.xcr0 & XFEATURE_MASK_PKRU) ||
> +            kvm_read_cr4_bits(vcpu, X86_CR4_PKE)))
> +               __write_pkru(vcpu->arch.pkru, false);

Please tell me I'm missing something (e.g. KVM very cleverly managing
the PKRU register using intercepts) that makes this reliably load the
guest value.  An innocent or malicious guest could easily make that
condition evaluate to false, thus allowing the host PKRU value to be
live in guest mode.  (Or is something fancy going on here?)

I don't even want to think about what happens if a perf NMI hits and
accesses host user memory while the guest PKRU is live (on VMX -- I
think this can't happen on SVM).

>  }
>  EXPORT_SYMBOL_GPL(kvm_load_guest_xsave_state);
>
> @@ -925,11 +925,11 @@ void kvm_load_host_xsave_state(struct kvm_vcpu *vcpu)
>                 return;
>
>         if (static_cpu_has(X86_FEATURE_PKU) &&
> -           (kvm_read_cr4_bits(vcpu, X86_CR4_PKE) ||
> -            (vcpu->arch.xcr0 & XFEATURE_MASK_PKRU))) {
> +           ((vcpu->arch.xcr0 & XFEATURE_MASK_PKRU) ||
> +            kvm_read_cr4_bits(vcpu, X86_CR4_PKE))) {
>                 vcpu->arch.pkru = rdpkru();
>                 if (vcpu->arch.pkru != vcpu->arch.host_pkru)
> -                       __write_pkru(vcpu->arch.host_pkru);
> +                       __write_pkru(vcpu->arch.host_pkru, true);
>         }

Suppose the guest writes to PKRU and then, without exiting, sets PKE =
0 and XCR0[PKRU] = 0.  (Or are the intercepts such that this can't
happen except on SEV where maybe SEV magic makes the problem go away?)

I admit I'm fairly mystified as to why KVM doesn't handle PKRU like
the rest of guest XSTATE.