[PATCH 0/3] KVM, pkeys: fix handling of PKRU across migration

[PATCH 0/3] KVM, pkeys: fix handling of PKRU across migration

[Paolo Bonzini]

The host pkru is restored right after vcpu exit (commit 1be0e61), so
KVM_GET_XSAVE will return the host PKRU value instead.  In general,
the PKRU value in vcpu->arch.guest_fpu.state cannot be trusted.

Series as follows:

1) fix independent bug which would cause an oops

2) remove an unnecessary abstraction

3) fix the bug

Please test the patches, as I don't have the affected hardware.  Note
that I need the results before tomorrow in order to send these patches
to Linus before going on vacation.

Thanks,

Paolo


Paolo Bonzini (3):
  KVM: x86: block guest protection keys unless the host has them enabled
  KVM: x86: simplify handling of PKRU
  KVM, pkeys: do not use PKRU value in vcpu->arch.guest_fpu.state

 arch/x86/include/asm/fpu/internal.h |  6 +++---
 arch/x86/include/asm/kvm_host.h     |  1 +
 arch/x86/kvm/cpuid.c                |  2 +-
 arch/x86/kvm/kvm_cache_regs.h       |  5 -----
 arch/x86/kvm/mmu.h                  |  2 +-
 arch/x86/kvm/svm.c                  |  7 -------
 arch/x86/kvm/vmx.c                  | 25 ++++++++-----------------
 arch/x86/kvm/x86.c                  | 17 ++++++++++++++---
 8 files changed, 28 insertions(+), 37 deletions(-)

-- 
1.8.3.1

[PATCH 1/3] KVM: x86: block guest protection keys unless the host has them enabled

[Paolo Bonzini]

If the host has protection keys disabled, we cannot read and write the
guest PKRU---RDPKRU and WRPKRU fail with #GP(0) if CR4.PKE=0.  Block
the PKU cpuid bit in that case.

This ensures that guest_CR4.PKE=1 implies host_CR4.PKE=1.

Fixes: 1be0e61c1f255faaeab04a390e00c8b9b9042870
Signed-off-by: Paolo Bonzini <pbonzini@redhat.com>
---
 arch/x86/kvm/cpuid.c | 2 +-
 1 file changed, 1 insertion(+), 1 deletion(-)

diff --git a/arch/x86/kvm/cpuid.c b/arch/x86/kvm/cpuid.c
index 59ca2eea522c..19adbb418443 100644
--- a/arch/x86/kvm/cpuid.c
+++ b/arch/x86/kvm/cpuid.c
@@ -469,7 +469,7 @@ static inline int __do_cpuid_ent(struct kvm_cpuid_entry2 *entry, u32 function,
 			entry->ecx &= kvm_cpuid_7_0_ecx_x86_features;
 			cpuid_mask(&entry->ecx, CPUID_7_ECX);
 			/* PKU is not yet implemented for shadow paging. */
-			if (!tdp_enabled)
+			if (!tdp_enabled || !boot_cpu_has(X86_FEATURE_OSPKE))
 				entry->ecx &= ~F(PKU);
 			entry->edx &= kvm_cpuid_7_0_edx_x86_features;
 			entry->edx &= get_scattered_cpuid_leaf(7, 0, CPUID_EDX);
-- 
1.8.3.1

[PATCH 2/3] KVM: x86: simplify handling of PKRU

[Paolo Bonzini]

Move it to struct kvm_arch_vcpu, replacing guest_pkru_valid with a
simple comparison against the host value of the register.  The write of
PKRU in addition can be skipped if the guest has not enabled the feature.
Once we do this, we need not test OSPKE in the host anymore, because
guest_CR4.PKE=1 implies host_CR4.PKE=1.

The static PKU test is kept to elide the code on older CPUs.

Suggested-by: Yang Zhang <zy107165@alibaba-inc.com>
Fixes: 1be0e61c1f255faaeab04a390e00c8b9b9042870
Signed-off-by: Paolo Bonzini <pbonzini@redhat.com>
---
 arch/x86/include/asm/kvm_host.h |  1 +
 arch/x86/kvm/kvm_cache_regs.h   |  5 -----
 arch/x86/kvm/mmu.h              |  2 +-
 arch/x86/kvm/svm.c              |  7 -------
 arch/x86/kvm/vmx.c              | 25 ++++++++-----------------
 5 files changed, 10 insertions(+), 30 deletions(-)

diff --git a/arch/x86/include/asm/kvm_host.h b/arch/x86/include/asm/kvm_host.h
index 87ac4fba6d8e..f4d120a3e22e 100644
--- a/arch/x86/include/asm/kvm_host.h
+++ b/arch/x86/include/asm/kvm_host.h
@@ -492,6 +492,7 @@ struct kvm_vcpu_arch {
 	unsigned long cr4;
 	unsigned long cr4_guest_owned_bits;
 	unsigned long cr8;
+	u32 pkru;
 	u32 hflags;
 	u64 efer;
 	u64 apic_base;
diff --git a/arch/x86/kvm/kvm_cache_regs.h b/arch/x86/kvm/kvm_cache_regs.h
index 762cdf2595f9..e1e89ee4af75 100644
--- a/arch/x86/kvm/kvm_cache_regs.h
+++ b/arch/x86/kvm/kvm_cache_regs.h
@@ -84,11 +84,6 @@ static inline u64 kvm_read_edx_eax(struct kvm_vcpu *vcpu)
 		| ((u64)(kvm_register_read(vcpu, VCPU_REGS_RDX) & -1u) << 32);
 }
 
-static inline u32 kvm_read_pkru(struct kvm_vcpu *vcpu)
-{
-	return kvm_x86_ops->get_pkru(vcpu);
-}
-
 static inline void enter_guest_mode(struct kvm_vcpu *vcpu)
 {
 	vcpu->arch.hflags |= HF_GUEST_MASK;
diff --git a/arch/x86/kvm/mmu.h b/arch/x86/kvm/mmu.h
index d7d248a000dd..4b9a3ae6b725 100644
--- a/arch/x86/kvm/mmu.h
+++ b/arch/x86/kvm/mmu.h
@@ -185,7 +185,7 @@ static inline u8 permission_fault(struct kvm_vcpu *vcpu, struct kvm_mmu *mmu,
 		* index of the protection domain, so pte_pkey * 2 is
 		* is the index of the first bit for the domain.
 		*/
-		pkru_bits = (kvm_read_pkru(vcpu) >> (pte_pkey * 2)) & 3;
+		pkru_bits = (vcpu->arch.pkru >> (pte_pkey * 2)) & 3;
 
 		/* clear present bit, replace PFEC.RSVD with ACC_USER_MASK. */
 		offset = (pfec & ~1) +
diff --git a/arch/x86/kvm/svm.c b/arch/x86/kvm/svm.c
index 1107626938cc..52e88bda35ea 100644
--- a/arch/x86/kvm/svm.c
+++ b/arch/x86/kvm/svm.c
@@ -1777,11 +1777,6 @@ static void svm_set_rflags(struct kvm_vcpu *vcpu, unsigned long rflags)
 	to_svm(vcpu)->vmcb->save.rflags = rflags;
 }
 
-static u32 svm_get_pkru(struct kvm_vcpu *vcpu)
-{
-	return 0;
-}
-
 static void svm_cache_reg(struct kvm_vcpu *vcpu, enum kvm_reg reg)
 {
 	switch (reg) {
@@ -5413,8 +5408,6 @@ static void svm_setup_mce(struct kvm_vcpu *vcpu)
 	.get_rflags = svm_get_rflags,
 	.set_rflags = svm_set_rflags,
 
-	.get_pkru = svm_get_pkru,
-
 	.tlb_flush = svm_flush_tlb,
 
 	.run = svm_vcpu_run,
diff --git a/arch/x86/kvm/vmx.c b/arch/x86/kvm/vmx.c
index 9b21b1223035..c6ef2940119b 100644
--- a/arch/x86/kvm/vmx.c
+++ b/arch/x86/kvm/vmx.c
@@ -636,8 +636,6 @@ struct vcpu_vmx {
 
 	u64 current_tsc_ratio;
 
-	bool guest_pkru_valid;
-	u32 guest_pkru;
 	u32 host_pkru;
 
 	/*
@@ -2383,11 +2381,6 @@ static void vmx_set_rflags(struct kvm_vcpu *vcpu, unsigned long rflags)
 		to_vmx(vcpu)->emulation_required = emulation_required(vcpu);
 }
 
-static u32 vmx_get_pkru(struct kvm_vcpu *vcpu)
-{
-	return to_vmx(vcpu)->guest_pkru;
-}
-
 static u32 vmx_get_interrupt_shadow(struct kvm_vcpu *vcpu)
 {
 	u32 interruptibility = vmcs_read32(GUEST_INTERRUPTIBILITY_INFO);
@@ -9020,8 +9013,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_PKU) &&
+	    kvm_read_cr4_bits(vcpu, X86_CR4_PKE) &&
+	    vcpu->arch.pkru != vmx->host_pkru)
+		__write_pkru(vcpu->arch.pkru);
 
 	atomic_switch_perf_msrs(vmx);
 	debugctlmsr = get_debugctlmsr();
@@ -9169,13 +9164,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_PKU) &&
+	    kvm_read_cr4_bits(vcpu, X86_CR4_PKE)) {
+		vcpu->arch.pkru = __read_pkru();
+		if (vcpu->arch.pkru != vmx->host_pkru)
 			__write_pkru(vmx->host_pkru);
-		} else
-			vmx->guest_pkru_valid = false;
 	}
 
 	/*
@@ -11682,8 +11675,6 @@ static void vmx_setup_mce(struct kvm_vcpu *vcpu)
 	.get_rflags = vmx_get_rflags,
 	.set_rflags = vmx_set_rflags,
 
-	.get_pkru = vmx_get_pkru,
-
 	.tlb_flush = vmx_flush_tlb,
 
 	.run = vmx_vcpu_run,
-- 
1.8.3.1

[PATCH 3/3] KVM, pkeys: do not use PKRU value in vcpu->arch.guest_fpu.state

[Paolo Bonzini]

The host pkru is restored right after vcpu exit (commit 1be0e61), so
KVM_GET_XSAVE will return the host PKRU value instead.  Fix this by
using the guest PKRU explicitly in fill_xsave and load_xsave.  This
part is based on a patch by Junkang Fu.

The host PKRU data may also not match the value in vcpu->arch.guest_fpu.state,
because it could have been changed by userspace since the last time
it was saved, so skip loading it in kvm_load_guest_fpu.

Reported-by: Junkang Fu <junkang.fjk@alibaba-inc.com>
Cc: Yang Zhang <zy107165@alibaba-inc.com>
Fixes: 1be0e61c1f255faaeab04a390e00c8b9b9042870
Signed-off-by: Paolo Bonzini <pbonzini@redhat.com>
---
 arch/x86/include/asm/fpu/internal.h |  6 +++---
 arch/x86/kvm/x86.c                  | 17 ++++++++++++++---
 2 files changed, 17 insertions(+), 6 deletions(-)

diff --git a/arch/x86/include/asm/fpu/internal.h b/arch/x86/include/asm/fpu/internal.h
index 255645f60ca2..554cdb205d17 100644
--- a/arch/x86/include/asm/fpu/internal.h
+++ b/arch/x86/include/asm/fpu/internal.h
@@ -450,10 +450,10 @@ static inline int copy_fpregs_to_fpstate(struct fpu *fpu)
 	return 0;
 }
 
-static inline void __copy_kernel_to_fpregs(union fpregs_state *fpstate)
+static inline void __copy_kernel_to_fpregs(union fpregs_state *fpstate, u64 mask)
 {
 	if (use_xsave()) {
-		copy_kernel_to_xregs(&fpstate->xsave, -1);
+		copy_kernel_to_xregs(&fpstate->xsave, mask);
 	} else {
 		if (use_fxsr())
 			copy_kernel_to_fxregs(&fpstate->fxsave);
@@ -477,7 +477,7 @@ static inline void copy_kernel_to_fpregs(union fpregs_state *fpstate)
 			: : [addr] "m" (fpstate));
 	}
 
-	__copy_kernel_to_fpregs(fpstate);
+	__copy_kernel_to_fpregs(fpstate, -1);
 }
 
 extern int copy_fpstate_to_sigframe(void __user *buf, void __user *fp, int size);
diff --git a/arch/x86/kvm/x86.c b/arch/x86/kvm/x86.c
index d734aa8c5b4f..05a5e57c6f39 100644
--- a/arch/x86/kvm/x86.c
+++ b/arch/x86/kvm/x86.c
@@ -3245,7 +3245,12 @@ static void fill_xsave(u8 *dest, struct kvm_vcpu *vcpu)
 			u32 size, offset, ecx, edx;
 			cpuid_count(XSTATE_CPUID, index,
 				    &size, &offset, &ecx, &edx);
-			memcpy(dest + offset, src, size);
+			if (feature == XFEATURE_MASK_PKRU)
+				memcpy(dest + offset, &vcpu->arch.pkru,
+				       sizeof(vcpu->arch.pkru));
+			else
+				memcpy(dest + offset, src, size);
+
 		}
 
 		valid -= feature;
@@ -3283,7 +3288,11 @@ static void load_xsave(struct kvm_vcpu *vcpu, u8 *src)
 			u32 size, offset, ecx, edx;
 			cpuid_count(XSTATE_CPUID, index,
 				    &size, &offset, &ecx, &edx);
-			memcpy(dest, src + offset, size);
+			if (feature == XFEATURE_MASK_PKRU)
+				memcpy(&vcpu->arch.pkru, src + offset,
+				       sizeof(vcpu->arch.pkru));
+			else
+				memcpy(dest, src + offset, size);
 		}
 
 		valid -= feature;
@@ -7633,7 +7642,9 @@ void kvm_load_guest_fpu(struct kvm_vcpu *vcpu)
 	 */
 	vcpu->guest_fpu_loaded = 1;
 	__kernel_fpu_begin();
-	__copy_kernel_to_fpregs(&vcpu->arch.guest_fpu.state);
+	/* PKRU is separately restored in kvm_x86_ops->run.  */
+	__copy_kernel_to_fpregs(&vcpu->arch.guest_fpu.state,
+				~XFEATURE_MASK_PKRU);
 	trace_kvm_fpu(1);
 }
 
-- 
1.8.3.1

Re: [PATCH 1/3] KVM: x86: block guest protection keys unless the host has them enabled

[David Hildenbrand]

On 24.08.2017 12:12, Paolo Bonzini wrote:
> If the host has protection keys disabled, we cannot read and write the
> guest PKRU---RDPKRU and WRPKRU fail with #GP(0) if CR4.PKE=0.  Block
> the PKU cpuid bit in that case.
> 
> This ensures that guest_CR4.PKE=1 implies host_CR4.PKE=1.
> 
> Fixes: 1be0e61c1f255faaeab04a390e00c8b9b9042870
> Signed-off-by: Paolo Bonzini <pbonzini@redhat.com>
> ---
>  arch/x86/kvm/cpuid.c | 2 +-
>  1 file changed, 1 insertion(+), 1 deletion(-)
> 
> diff --git a/arch/x86/kvm/cpuid.c b/arch/x86/kvm/cpuid.c
> index 59ca2eea522c..19adbb418443 100644
> --- a/arch/x86/kvm/cpuid.c
> +++ b/arch/x86/kvm/cpuid.c
> @@ -469,7 +469,7 @@ static inline int __do_cpuid_ent(struct kvm_cpuid_entry2 *entry, u32 function,
>  			entry->ecx &= kvm_cpuid_7_0_ecx_x86_features;
>  			cpuid_mask(&entry->ecx, CPUID_7_ECX);
>  			/* PKU is not yet implemented for shadow paging. */
> -			if (!tdp_enabled)
> +			if (!tdp_enabled || !boot_cpu_has(X86_FEATURE_OSPKE))
>  				entry->ecx &= ~F(PKU);
>  			entry->edx &= kvm_cpuid_7_0_edx_x86_features;
>  			entry->edx &= get_scattered_cpuid_leaf(7, 0, CPUID_EDX);
> 

Reviewed-by: David Hildenbrand <david@redhat.com>

-- 

Thanks,

David

Re: [PATCH 2/3] KVM: x86: simplify handling of PKRU

[David Hildenbrand]

On 24.08.2017 12:12, Paolo Bonzini wrote:
> Move it to struct kvm_arch_vcpu, replacing guest_pkru_valid with a
> simple comparison against the host value of the register.  The write of
> PKRU in addition can be skipped if the guest has not enabled the feature.
> Once we do this, we need not test OSPKE in the host anymore, because
> guest_CR4.PKE=1 implies host_CR4.PKE=1.
> 
> The static PKU test is kept to elide the code on older CPUs.

>From what I can tell, this looks good to me!


-- 

Thanks,

David

Re: [PATCH 0/3] KVM, pkeys: fix handling of PKRU across migration

[Yang Zhang]

On 2017/8/24 18:12, Paolo Bonzini wrote:
> The host pkru is restored right after vcpu exit (commit 1be0e61), so
> KVM_GET_XSAVE will return the host PKRU value instead.  In general,
> the PKRU value in vcpu->arch.guest_fpu.state cannot be trusted.
> 
> Series as follows:
> 
> 1) fix independent bug which would cause an oops
> 
> 2) remove an unnecessary abstraction
> 
> 3) fix the bug
> 
> Please test the patches, as I don't have the affected hardware.  Note
> that I need the results before tomorrow in order to send these patches
> to Linus before going on vacation.

hi Quan

Can you help to test Paolo's patch?

> 
> Thanks,
> 
> Paolo
> 
> 
> Paolo Bonzini (3):
>    KVM: x86: block guest protection keys unless the host has them enabled
>    KVM: x86: simplify handling of PKRU
>    KVM, pkeys: do not use PKRU value in vcpu->arch.guest_fpu.state
> 
>   arch/x86/include/asm/fpu/internal.h |  6 +++---
>   arch/x86/include/asm/kvm_host.h     |  1 +
>   arch/x86/kvm/cpuid.c                |  2 +-
>   arch/x86/kvm/kvm_cache_regs.h       |  5 -----
>   arch/x86/kvm/mmu.h                  |  2 +-
>   arch/x86/kvm/svm.c                  |  7 -------
>   arch/x86/kvm/vmx.c                  | 25 ++++++++-----------------
>   arch/x86/kvm/x86.c                  | 17 ++++++++++++++---
>   8 files changed, 28 insertions(+), 37 deletions(-)
> 


-- 
Yang
Alibaba Cloud Computing

Re: [PATCH 0/3] KVM, pkeys: fix handling of PKRU across migration

[Yang Zhang]

On 2017/8/24 18:12, Paolo Bonzini wrote:
> The host pkru is restored right after vcpu exit (commit 1be0e61), so
> KVM_GET_XSAVE will return the host PKRU value instead.  In general,
> the PKRU value in vcpu->arch.guest_fpu.state cannot be trusted.
> 
> Series as follows:
> 
> 1) fix independent bug which would cause an oops
> 
> 2) remove an unnecessary abstraction
> 
> 3) fix the bug
> 
> Please test the patches, as I don't have the affected hardware.  Note
> that I need the results before tomorrow in order to send these patches
> to Linus before going on vacation.
> 
> Thanks,
> 
> Paolo
> 
> 
> Paolo Bonzini (3):
>    KVM: x86: block guest protection keys unless the host has them enabled
>    KVM: x86: simplify handling of PKRU
>    KVM, pkeys: do not use PKRU value in vcpu->arch.guest_fpu.state
> 
>   arch/x86/include/asm/fpu/internal.h |  6 +++---
>   arch/x86/include/asm/kvm_host.h     |  1 +
>   arch/x86/kvm/cpuid.c                |  2 +-
>   arch/x86/kvm/kvm_cache_regs.h       |  5 -----
>   arch/x86/kvm/mmu.h                  |  2 +-
>   arch/x86/kvm/svm.c                  |  7 -------
>   arch/x86/kvm/vmx.c                  | 25 ++++++++-----------------
>   arch/x86/kvm/x86.c                  | 17 ++++++++++++++---
>   8 files changed, 28 insertions(+), 37 deletions(-)
> 

Reviewed-by: Yang Zhang <yang.zhang.wz@gmail.com>

-- 
Yang
Alibaba Cloud Computing

Re: [PATCH 0/3] KVM, pkeys: fix handling of PKRU across migration

[Paolo Bonzini]

On 28/08/2017 03:32, Quan Xu wrote:
> 
> Paolo， I have verified this patch set..

Thanks!  In the end I managed to use QEMU's SVM emulation to convince
myself that the code was correct, so it is already included in Linux 4.13.

Paolo

> Tested-by: Quan Xu <quan.xu0@gmail.com <mailto:quan.xu0@gmail.com>>
> 
> 2017-08-25 8:43 GMT+08:00 Yang Zhang <yang.zhang.wz@gmail.com
> <mailto:yang.zhang.wz@gmail.com>>:
> 
>     On 2017/8/24 18:12, Paolo Bonzini wrote:
> 
>         The host pkru is restored right after vcpu exit (commit 1be0e61), so
>         KVM_GET_XSAVE will return the host PKRU value instead.  In general,
>         the PKRU value in vcpu->arch.guest_fpu.state cannot be trusted.
> 
>         Series as follows:
> 
>         1) fix independent bug which would cause an oops
> 
>         2) remove an unnecessary abstraction
> 
>         3) fix the bug
> 
>         Please test the patches, as I don't have the affected hardware. 
>         Note
>         that I need the results before tomorrow in order to send these
>         patches
>         to Linus before going on vacation.
> 
> 
>     hi Quan
> 
>     Can you help to test Paolo's patch?
> 
> 
>         Thanks,
> 
>         Paolo
> 
> 
>         Paolo Bonzini (3):
>            KVM: x86: block guest protection keys unless the host has
>         them enabled
>            KVM: x86: simplify handling of PKRU
>            KVM, pkeys: do not use PKRU value in vcpu->arch.guest_fpu.state
> 
>           arch/x86/include/asm/fpu/internal.h |  6 +++---
>           arch/x86/include/asm/kvm_host.h     |  1 +
>           arch/x86/kvm/cpuid.c                |  2 +-
>           arch/x86/kvm/kvm_cache_regs.h       |  5 -----
>           arch/x86/kvm/mmu.h                  |  2 +-
>           arch/x86/kvm/svm.c                  |  7 -------
>           arch/x86/kvm/vmx.c                  | 25 ++++++++-----------------
>           arch/x86/kvm/x86.c                  | 17 ++++++++++++++---
>           8 files changed, 28 insertions(+), 37 deletions(-)
> 
> 
> 
>     -- 
>     Yang
>     Alibaba Cloud Computing
> 
>