linux-kernel - Re: [PATCH 1/2] KVM: x86: simplify handling of PKRU

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: <59e6c0e2-6422-7803-5a0f-b3c2b00edb26@gmail.com>
Date:   Thu, 24 Aug 2017 18:05:59 +0800
From:   Yang Zhang <yang.zhang.wz@...il.com>
To:     Paolo Bonzini <pbonzini@...hat.com>, linux-kernel@...r.kernel.org,
        kvm@...r.kernel.org
Cc:     junkang.fjk@...baba-inc.com
Subject: Re: [PATCH 1/2] KVM: x86: simplify handling of PKRU

On 2017/8/24 17:19, Paolo Bonzini wrote:
> On 24/08/2017 11:09, Yang Zhang wrote:
>>> +    if (static_cpu_has(X86_FEATURE_OSPKE) &&
>>
>> We expose protection key to VM without check whether OSPKE is enabled or
>> not. Why not check guest's cpuid here which also can avoid unnecessary
>> access to pkru?
> 
> Checking guest CPUID is pretty slow.  We could check CR4.PKE though.
> 
> Also, using static_cpu_has with OSPKE is probably wrong.  But if we do
> check CR4.PKE, we can check X86_FEATURE_PKU instead, so something like
> 
> 	if (static_cpu_has(X86_FEATURE_PKU) &&
> 	    kvm_read_cr4_bits(vcpu, X86_CR4_PKE) &&
> 	    vcpu->arch.pkru != vmx->host_pkru)
> 
> ... but then, kvm_read_cr4_bits is also pretty slow---and we don't
> really need it, since all CR4 writes cause a vmexit.  So for now I'd
> stay with this patch, only s/static_cpu_has/boot_cpu_has/g.
> 
> Of course you can send improvements on top!

ok, since most OS distributions don't support protection key so far 
which means vcpu->arch.pkru always 0 in it and i remember host_pkru will 
be set to 55555554 be default. To avoid the unnecessary access to pkru, 
how about the following change:

@@ -4338,6 +4331,9 @@ static int vmx_set_cr4(struct kvm_vcpu *vcpu, 
unsigned long cr4)
                         return 1;
         }

+       if (cr4 & X86_CR4_PKE)
+               to_vmx(vcpu)->guest_pkru_valid = true;
+
         if (to_vmx(vcpu)->nested.vmxon && !nested_cr4_valid(vcpu, cr4))
                 return 1;

@@ -9020,8 +9016,10 @@ static void __noclone vmx_vcpu_run(struct 
kvm_vcpu *vcpu)
         if (vcpu->guest_debug & KVM_GUESTDBG_SINGLESTEP)
                 vmx_set_interrupt_shadow(vcpu, 0);

-       if (vmx->guest_pkru_valid)
-               __write_pkru(vmx->guest_pkru);
+       if (static_cpu_has(X86_FEATURE_OSPKE) &&
+           vmx->guest_pkru_valid &&
+           vcpu->arch.pkru != vmx->host_pkru)
+               __write_pkru(vcpu->arch.pkru);

         atomic_switch_perf_msrs(vmx);
         debugctlmsr = get_debugctlmsr();
@@ -9169,13 +9167,11 @@ static void __noclone vmx_vcpu_run(struct 
kvm_vcpu *vcpu)
          * back on host, so it is safe to read guest PKRU from current
          * XSAVE.
          */
-       if (boot_cpu_has(X86_FEATURE_OSPKE)) {
-               vmx->guest_pkru = __read_pkru();
-               if (vmx->guest_pkru != vmx->host_pkru) {
-                       vmx->guest_pkru_valid = true;
+       if (static_cpu_has(X86_FEATURE_OSPKE) &&
+           vmx->guest_pkru_valid) {
+               vcpu->arch.pkru = __read_pkru();
+               if (vcpu->arch.pkru != vmx->host_pkru)
                         __write_pkru(vmx->host_pkru);
-               } else
-                       vmx->guest_pkru_valid = false;
         }

         /*

-- 
Yang
Alibaba Cloud Computing