All of lore.kernel.org
 help / color / mirror / Atom feed
* [PATCH 0/3] KVM: x86: VM alloc bug fix and cleanup
@ 2020-01-27  0:41 Sean Christopherson
  2020-01-27  0:41 ` [PATCH 1/3] KVM: x86: Gracefully handle __vmalloc() failure during VM allocation Sean Christopherson
                   ` (3 more replies)
  0 siblings, 4 replies; 6+ messages in thread
From: Sean Christopherson @ 2020-01-27  0:41 UTC (permalink / raw)
  To: Paolo Bonzini
  Cc: Sean Christopherson, Vitaly Kuznetsov, Wanpeng Li, Jim Mattson,
	Joerg Roedel, kvm, linux-kernel

Fix a (fairly) long standing NULL pointer dereference if VM allocation
fails, and do a bit of clean up on top.

I would have preferred to omit patch 01, i.e. fix the bug via patch 02,
but unfortunately (long term support) kernel 4.19 doesn't have the
accounting changes, which would make backporting the fix extra annoying
for no real benefit.

Sean Christopherson (3):
  KVM: x86: Gracefully handle __vmalloc() failure during VM allocation
  KVM: x86: Directly return __vmalloc() result in ->vm_alloc()
  KVM: x86: Consolidate VM allocation and free for VMX and SVM

 arch/x86/include/asm/kvm_host.h | 12 ++++--------
 arch/x86/kvm/svm.c              | 15 ++++-----------
 arch/x86/kvm/vmx/vmx.c          | 16 ++++------------
 arch/x86/kvm/x86.c              |  7 +++++++
 4 files changed, 19 insertions(+), 31 deletions(-)

-- 
2.24.1


^ permalink raw reply	[flat|nested] 6+ messages in thread
* [PATCH 1/3] KVM: x86: Gracefully handle __vmalloc() failure during VM allocation
  2020-01-27  0:41 [PATCH 0/3] KVM: x86: VM alloc bug fix and cleanup Sean Christopherson
@ 2020-01-27  0:41 ` Sean Christopherson
  2020-01-27  0:41 ` [PATCH 2/3] KVM: x86: Directly return __vmalloc() result in ->vm_alloc() Sean Christopherson
                   ` (2 subsequent siblings)
  3 siblings, 0 replies; 6+ messages in thread
From: Sean Christopherson @ 2020-01-27  0:41 UTC (permalink / raw)
  To: Paolo Bonzini
  Cc: Sean Christopherson, Vitaly Kuznetsov, Wanpeng Li, Jim Mattson,
	Joerg Roedel, kvm, linux-kernel

Check the result of __vmalloc() to avoid dereferencing a NULL pointer in
the event that allocation failres.

Fixes: 40bbb9d03f05d ("KVM: VMX: add struct kvm_vmx to hold VMX specific KVM vars")
Fixes: 81811c162d4da ("KVM: SVM: add struct kvm_svm to hold SVM specific KVM vars")
Fixes: d1e5b0e98ea27 ("kvm: Make VM ioctl do valloc for some archs")
Cc: stable@vger.kernel.org
Signed-off-by: Sean Christopherson <sean.j.christopherson@intel.com>
---
 arch/x86/kvm/svm.c     | 4 ++++
 arch/x86/kvm/vmx/vmx.c | 4 ++++
 2 files changed, 8 insertions(+)

diff --git a/arch/x86/kvm/svm.c b/arch/x86/kvm/svm.c
index bf0556588ad0..6565257dea39 100644
--- a/arch/x86/kvm/svm.c
+++ b/arch/x86/kvm/svm.c
@@ -1951,6 +1951,10 @@ static struct kvm *svm_vm_alloc(void)
 	struct kvm_svm *kvm_svm = __vmalloc(sizeof(struct kvm_svm),
 					    GFP_KERNEL_ACCOUNT | __GFP_ZERO,
 					    PAGE_KERNEL);
+
+	if (!kvm_svm)
+		return NULL;
+
 	return &kvm_svm->kvm;
 }
 
diff --git a/arch/x86/kvm/vmx/vmx.c b/arch/x86/kvm/vmx/vmx.c
index 1419c53aed16..45f3f215e9df 100644
--- a/arch/x86/kvm/vmx/vmx.c
+++ b/arch/x86/kvm/vmx/vmx.c
@@ -6662,6 +6662,10 @@ static struct kvm *vmx_vm_alloc(void)
 	struct kvm_vmx *kvm_vmx = __vmalloc(sizeof(struct kvm_vmx),
 					    GFP_KERNEL_ACCOUNT | __GFP_ZERO,
 					    PAGE_KERNEL);
+
+	if (!kvm_vmx)
+		return NULL;
+
 	return &kvm_vmx->kvm;
 }
 
-- 
2.24.1


^ permalink raw reply related	[flat|nested] 6+ messages in thread
* [PATCH 2/3] KVM: x86: Directly return __vmalloc() result in ->vm_alloc()
  2020-01-27  0:41 [PATCH 0/3] KVM: x86: VM alloc bug fix and cleanup Sean Christopherson
  2020-01-27  0:41 ` [PATCH 1/3] KVM: x86: Gracefully handle __vmalloc() failure during VM allocation Sean Christopherson
@ 2020-01-27  0:41 ` Sean Christopherson
  2020-01-27  0:41 ` [PATCH 3/3] KVM: x86: Consolidate VM allocation and free for VMX and SVM Sean Christopherson
  2020-01-27 15:48 ` [PATCH 0/3] KVM: x86: VM alloc bug fix and cleanup Vitaly Kuznetsov
  3 siblings, 0 replies; 6+ messages in thread
From: Sean Christopherson @ 2020-01-27  0:41 UTC (permalink / raw)
  To: Paolo Bonzini
  Cc: Sean Christopherson, Vitaly Kuznetsov, Wanpeng Li, Jim Mattson,
	Joerg Roedel, kvm, linux-kernel

Directly return the __vmalloc() result in {svm,vmx}_vm_alloc() to pave
the way for handling VM alloc/free in common x86 code, and to obviate
the need to check the result of __vmalloc() in vendor specific code.
Add a build-time assertion to ensure each structs' "kvm" field stays at
offset 0, which allows interpreting a "struct kvm_{svm,vmx}" as a
"struct kvm".

Signed-off-by: Sean Christopherson <sean.j.christopherson@intel.com>
---
 arch/x86/kvm/svm.c     | 12 ++++--------
 arch/x86/kvm/vmx/vmx.c | 12 ++++--------
 2 files changed, 8 insertions(+), 16 deletions(-)

diff --git a/arch/x86/kvm/svm.c b/arch/x86/kvm/svm.c
index 6565257dea39..4fff99722487 100644
--- a/arch/x86/kvm/svm.c
+++ b/arch/x86/kvm/svm.c
@@ -1948,19 +1948,15 @@ static void __unregister_enc_region_locked(struct kvm *kvm,
 
 static struct kvm *svm_vm_alloc(void)
 {
-	struct kvm_svm *kvm_svm = __vmalloc(sizeof(struct kvm_svm),
-					    GFP_KERNEL_ACCOUNT | __GFP_ZERO,
-					    PAGE_KERNEL);
+	BUILD_BUG_ON(offsetof(struct kvm_svm, kvm) != 0);
 
-	if (!kvm_svm)
-		return NULL;
-
-	return &kvm_svm->kvm;
+	return __vmalloc(sizeof(struct kvm_svm),
+			 GFP_KERNEL_ACCOUNT | __GFP_ZERO, PAGE_KERNEL);
 }
 
 static void svm_vm_free(struct kvm *kvm)
 {
-	vfree(to_kvm_svm(kvm));
+	vfree(kvm);
 }
 
 static void sev_vm_destroy(struct kvm *kvm)
diff --git a/arch/x86/kvm/vmx/vmx.c b/arch/x86/kvm/vmx/vmx.c
index 45f3f215e9df..17e449330c8a 100644
--- a/arch/x86/kvm/vmx/vmx.c
+++ b/arch/x86/kvm/vmx/vmx.c
@@ -6659,20 +6659,16 @@ static void vmx_vcpu_run(struct kvm_vcpu *vcpu)
 
 static struct kvm *vmx_vm_alloc(void)
 {
-	struct kvm_vmx *kvm_vmx = __vmalloc(sizeof(struct kvm_vmx),
-					    GFP_KERNEL_ACCOUNT | __GFP_ZERO,
-					    PAGE_KERNEL);
+	BUILD_BUG_ON(offsetof(struct kvm_vmx, kvm) != 0);
 
-	if (!kvm_vmx)
-		return NULL;
-
-	return &kvm_vmx->kvm;
+	return __vmalloc(sizeof(struct kvm_vmx),
+			 GFP_KERNEL_ACCOUNT | __GFP_ZERO, PAGE_KERNEL);
 }
 
 static void vmx_vm_free(struct kvm *kvm)
 {
 	kfree(kvm->arch.hyperv.hv_pa_pg);
-	vfree(to_kvm_vmx(kvm));
+	vfree(kvm);
 }
 
 static void vmx_free_vcpu(struct kvm_vcpu *vcpu)
-- 
2.24.1


^ permalink raw reply related	[flat|nested] 6+ messages in thread
* [PATCH 3/3] KVM: x86: Consolidate VM allocation and free for VMX and SVM
  2020-01-27  0:41 [PATCH 0/3] KVM: x86: VM alloc bug fix and cleanup Sean Christopherson
  2020-01-27  0:41 ` [PATCH 1/3] KVM: x86: Gracefully handle __vmalloc() failure during VM allocation Sean Christopherson
  2020-01-27  0:41 ` [PATCH 2/3] KVM: x86: Directly return __vmalloc() result in ->vm_alloc() Sean Christopherson
@ 2020-01-27  0:41 ` Sean Christopherson
  2020-02-23  9:09   ` Paolo Bonzini
  2020-01-27 15:48 ` [PATCH 0/3] KVM: x86: VM alloc bug fix and cleanup Vitaly Kuznetsov
  3 siblings, 1 reply; 6+ messages in thread
From: Sean Christopherson @ 2020-01-27  0:41 UTC (permalink / raw)
  To: Paolo Bonzini
  Cc: Sean Christopherson, Vitaly Kuznetsov, Wanpeng Li, Jim Mattson,
	Joerg Roedel, kvm, linux-kernel

Move the VM allocation and free code to common x86 as the logic is
more or less identical across SVM and VMX.

Note, although hyperv.hv_pa_pg is part of the common kvm->arch, it's
(currently) only allocated by VMX VMs.  But, since kfree() plays nice
when passed a NULL pointer, the superfluous call for SVM is harmless
and avoids future churn if SVM gains support for HyperV's direct TLB
flush.

Signed-off-by: Sean Christopherson <sean.j.christopherson@intel.com>
---
 arch/x86/include/asm/kvm_host.h | 12 ++++--------
 arch/x86/kvm/svm.c              | 13 +++----------
 arch/x86/kvm/vmx/vmx.c          | 14 +++-----------
 arch/x86/kvm/x86.c              |  7 +++++++
 4 files changed, 17 insertions(+), 29 deletions(-)

diff --git a/arch/x86/include/asm/kvm_host.h b/arch/x86/include/asm/kvm_host.h
index 77d206a93658..06f48d3efa0a 100644
--- a/arch/x86/include/asm/kvm_host.h
+++ b/arch/x86/include/asm/kvm_host.h
@@ -1053,8 +1053,7 @@ struct kvm_x86_ops {
 	bool (*has_emulated_msr)(int index);
 	void (*cpuid_update)(struct kvm_vcpu *vcpu);
 
-	struct kvm *(*vm_alloc)(void);
-	void (*vm_free)(struct kvm *);
+	unsigned int (*vm_size)(void);
 	int (*vm_init)(struct kvm *kvm);
 	void (*vm_destroy)(struct kvm *kvm);
 
@@ -1271,13 +1270,10 @@ extern struct kmem_cache *x86_fpu_cache;
 #define __KVM_HAVE_ARCH_VM_ALLOC
 static inline struct kvm *kvm_arch_alloc_vm(void)
 {
-	return kvm_x86_ops->vm_alloc();
-}
-
-static inline void kvm_arch_free_vm(struct kvm *kvm)
-{
-	return kvm_x86_ops->vm_free(kvm);
+	return __vmalloc(kvm_x86_ops->vm_size(),
+			 GFP_KERNEL_ACCOUNT | __GFP_ZERO, PAGE_KERNEL);
 }
+void kvm_arch_free_vm(struct kvm *kvm);
 
 #define __KVM_HAVE_ARCH_FLUSH_REMOTE_TLB
 static inline int kvm_arch_flush_remote_tlb(struct kvm *kvm)
diff --git a/arch/x86/kvm/svm.c b/arch/x86/kvm/svm.c
index 4fff99722487..e72e209e7254 100644
--- a/arch/x86/kvm/svm.c
+++ b/arch/x86/kvm/svm.c
@@ -1946,17 +1946,11 @@ static void __unregister_enc_region_locked(struct kvm *kvm,
 	kfree(region);
 }
 
-static struct kvm *svm_vm_alloc(void)
+static unsigned int svm_vm_size(void)
 {
 	BUILD_BUG_ON(offsetof(struct kvm_svm, kvm) != 0);
 
-	return __vmalloc(sizeof(struct kvm_svm),
-			 GFP_KERNEL_ACCOUNT | __GFP_ZERO, PAGE_KERNEL);
-}
-
-static void svm_vm_free(struct kvm *kvm)
-{
-	vfree(kvm);
+	return sizeof(struct kvm_svm);
 }
 
 static void sev_vm_destroy(struct kvm *kvm)
@@ -7385,8 +7379,7 @@ static struct kvm_x86_ops svm_x86_ops __ro_after_init = {
 	.vcpu_free = svm_free_vcpu,
 	.vcpu_reset = svm_vcpu_reset,
 
-	.vm_alloc = svm_vm_alloc,
-	.vm_free = svm_vm_free,
+	.vm_size = svm_vm_size,
 	.vm_init = svm_vm_init,
 	.vm_destroy = svm_vm_destroy,
 
diff --git a/arch/x86/kvm/vmx/vmx.c b/arch/x86/kvm/vmx/vmx.c
index 17e449330c8a..17ae291122e5 100644
--- a/arch/x86/kvm/vmx/vmx.c
+++ b/arch/x86/kvm/vmx/vmx.c
@@ -6657,18 +6657,11 @@ static void vmx_vcpu_run(struct kvm_vcpu *vcpu)
 	vmx_complete_interrupts(vmx);
 }
 
-static struct kvm *vmx_vm_alloc(void)
+static unsigned int vmx_vm_size(void)
 {
 	BUILD_BUG_ON(offsetof(struct kvm_vmx, kvm) != 0);
 
-	return __vmalloc(sizeof(struct kvm_vmx),
-			 GFP_KERNEL_ACCOUNT | __GFP_ZERO, PAGE_KERNEL);
-}
-
-static void vmx_vm_free(struct kvm *kvm)
-{
-	kfree(kvm->arch.hyperv.hv_pa_pg);
-	vfree(kvm);
+	return sizeof(struct kvm_vmx);
 }
 
 static void vmx_free_vcpu(struct kvm_vcpu *vcpu)
@@ -7756,9 +7749,8 @@ static struct kvm_x86_ops vmx_x86_ops __ro_after_init = {
 	.cpu_has_accelerated_tpr = report_flexpriority,
 	.has_emulated_msr = vmx_has_emulated_msr,
 
+	.vm_size = vmx_vm_size,
 	.vm_init = vmx_vm_init,
-	.vm_alloc = vmx_vm_alloc,
-	.vm_free = vmx_vm_free,
 
 	.vcpu_create = vmx_create_vcpu,
 	.vcpu_free = vmx_free_vcpu,
diff --git a/arch/x86/kvm/x86.c b/arch/x86/kvm/x86.c
index 7e3f1d937224..f57a0bcd0e45 100644
--- a/arch/x86/kvm/x86.c
+++ b/arch/x86/kvm/x86.c
@@ -9653,6 +9653,13 @@ void kvm_arch_sched_in(struct kvm_vcpu *vcpu, int cpu)
 	kvm_x86_ops->sched_in(vcpu, cpu);
 }
 
+void kvm_arch_free_vm(struct kvm *kvm)
+{
+	kfree(kvm->arch.hyperv.hv_pa_pg);
+	vfree(kvm);
+}
+
+
 int kvm_arch_init_vm(struct kvm *kvm, unsigned long type)
 {
 	if (type)
-- 
2.24.1


^ permalink raw reply related	[flat|nested] 6+ messages in thread
* Re: [PATCH 0/3] KVM: x86: VM alloc bug fix and cleanup
  2020-01-27  0:41 [PATCH 0/3] KVM: x86: VM alloc bug fix and cleanup Sean Christopherson
                   ` (2 preceding siblings ...)
  2020-01-27  0:41 ` [PATCH 3/3] KVM: x86: Consolidate VM allocation and free for VMX and SVM Sean Christopherson
@ 2020-01-27 15:48 ` Vitaly Kuznetsov
  3 siblings, 0 replies; 6+ messages in thread
From: Vitaly Kuznetsov @ 2020-01-27 15:48 UTC (permalink / raw)
  To: Sean Christopherson
  Cc: Wanpeng Li, Jim Mattson, Joerg Roedel, kvm, linux-kernel, Paolo Bonzini

Sean Christopherson <sean.j.christopherson@intel.com> writes:

> Fix a (fairly) long standing NULL pointer dereference if VM allocation
> fails, and do a bit of clean up on top.
>
> I would have preferred to omit patch 01, i.e. fix the bug via patch 02,
> but unfortunately (long term support) kernel 4.19 doesn't have the
> accounting changes, which would make backporting the fix extra annoying
> for no real benefit.
>
> Sean Christopherson (3):
>   KVM: x86: Gracefully handle __vmalloc() failure during VM allocation
>   KVM: x86: Directly return __vmalloc() result in ->vm_alloc()
>   KVM: x86: Consolidate VM allocation and free for VMX and SVM
>

Reviewed-by: Vitaly Kuznetsov <vkuznets@redhat.com>

-- 
Vitaly


^ permalink raw reply	[flat|nested] 6+ messages in thread
* Re: [PATCH 3/3] KVM: x86: Consolidate VM allocation and free for VMX and SVM
  2020-01-27  0:41 ` [PATCH 3/3] KVM: x86: Consolidate VM allocation and free for VMX and SVM Sean Christopherson
@ 2020-02-23  9:09   ` Paolo Bonzini
  0 siblings, 0 replies; 6+ messages in thread
From: Paolo Bonzini @ 2020-02-23  9:09 UTC (permalink / raw)
  To: Sean Christopherson
  Cc: Vitaly Kuznetsov, Wanpeng Li, Jim Mattson, Joerg Roedel, kvm,
	linux-kernel

On 27/01/20 01:41, Sean Christopherson wrote:
> Move the VM allocation and free code to common x86 as the logic is
> more or less identical across SVM and VMX.
> 
> Note, although hyperv.hv_pa_pg is part of the common kvm->arch, it's
> (currently) only allocated by VMX VMs.  But, since kfree() plays nice
> when passed a NULL pointer, the superfluous call for SVM is harmless
> and avoids future churn if SVM gains support for HyperV's direct TLB
> flush.
> 
> Signed-off-by: Sean Christopherson <sean.j.christopherson@intel.com>

Queued, thanks.  Might as well make vm_size a field instead of a pointer 
to function, sacrificing the BUILD_BUG_ONs:

diff --git a/arch/x86/include/asm/kvm_host.h b/arch/x86/include/asm/kvm_host.h
index 24b87e2691c5..f8b45cc0bf49 100644
--- a/arch/x86/include/asm/kvm_host.h
+++ b/arch/x86/include/asm/kvm_host.h
@@ -1059,7 +1059,7 @@ struct kvm_x86_ops {
 	bool (*has_emulated_msr)(int index);
 	void (*cpuid_update)(struct kvm_vcpu *vcpu);
 
-	unsigned int (*vm_size)(void);
+	unsigned int vm_size;
 	int (*vm_init)(struct kvm *kvm);
 	void (*vm_destroy)(struct kvm *kvm);
 
@@ -1276,7 +1276,7 @@ struct kvm_arch_async_pf {
 #define __KVM_HAVE_ARCH_VM_ALLOC
 static inline struct kvm *kvm_arch_alloc_vm(void)
 {
-	return __vmalloc(kvm_x86_ops->vm_size(),
+	return __vmalloc(kvm_x86_ops->vm_size,
 			 GFP_KERNEL_ACCOUNT | __GFP_ZERO, PAGE_KERNEL);
 }
 void kvm_arch_free_vm(struct kvm *kvm);
diff --git a/arch/x86/kvm/svm.c b/arch/x86/kvm/svm.c
index a5a136e986e9..660387d6caf0 100644
--- a/arch/x86/kvm/svm.c
+++ b/arch/x86/kvm/svm.c
@@ -1955,13 +1955,6 @@ static void __unregister_enc_region_locked(struct kvm *kvm,
 	kfree(region);
 }
 
-static unsigned int svm_vm_size(void)
-{
-	BUILD_BUG_ON(offsetof(struct kvm_svm, kvm) != 0);
-
-	return sizeof(struct kvm_svm);
-}
-
 static void sev_vm_destroy(struct kvm *kvm)
 {
 	struct kvm_sev_info *sev = &to_kvm_svm(kvm)->sev_info;
@@ -7399,7 +7392,7 @@ static void svm_pre_update_apicv_exec_ctrl(struct kvm *kvm, bool activate)
 	.vcpu_free = svm_free_vcpu,
 	.vcpu_reset = svm_vcpu_reset,
 
-	.vm_size = svm_vm_size,
+	.vm_size = sizeof(struct kvm_svm),
 	.vm_init = svm_vm_init,
 	.vm_destroy = svm_vm_destroy,
 
diff --git a/arch/x86/kvm/vmx/vmx.c b/arch/x86/kvm/vmx/vmx.c
index 39a4fea03df5..57ac585394b9 100644
--- a/arch/x86/kvm/vmx/vmx.c
+++ b/arch/x86/kvm/vmx/vmx.c
@@ -6645,13 +6645,6 @@ static void vmx_vcpu_run(struct kvm_vcpu *vcpu)
 	vmx_complete_interrupts(vmx);
 }
 
-static unsigned int vmx_vm_size(void)
-{
-	BUILD_BUG_ON(offsetof(struct kvm_vmx, kvm) != 0);
-
-	return sizeof(struct kvm_vmx);
-}
-
 static void vmx_free_vcpu(struct kvm_vcpu *vcpu)
 {
 	struct vcpu_vmx *vmx = to_vmx(vcpu);
@@ -7749,7 +7742,7 @@ static bool vmx_check_apicv_inhibit_reasons(ulong bit)
 	.cpu_has_accelerated_tpr = report_flexpriority,
 	.has_emulated_msr = vmx_has_emulated_msr,
 
-	.vm_size = vmx_vm_size,
+	.vm_size = sizeof(struct kvm_vmx),
 	.vm_init = vmx_vm_init,
 
 	.vcpu_create = vmx_create_vcpu,


^ permalink raw reply related	[flat|nested] 6+ messages in thread
end of thread, other threads:[~2020-02-23  9:10 UTC | newest]

Thread overview: 6+ messages (download: mbox.gz / follow: Atom feed)
-- links below jump to the message on this page --
2020-01-27  0:41 [PATCH 0/3] KVM: x86: VM alloc bug fix and cleanup Sean Christopherson
2020-01-27  0:41 ` [PATCH 1/3] KVM: x86: Gracefully handle __vmalloc() failure during VM allocation Sean Christopherson
2020-01-27  0:41 ` [PATCH 2/3] KVM: x86: Directly return __vmalloc() result in ->vm_alloc() Sean Christopherson
2020-01-27  0:41 ` [PATCH 3/3] KVM: x86: Consolidate VM allocation and free for VMX and SVM Sean Christopherson
2020-02-23  9:09   ` Paolo Bonzini
2020-01-27 15:48 ` [PATCH 0/3] KVM: x86: VM alloc bug fix and cleanup Vitaly Kuznetsov

This is an external index of several public inboxes,
see mirroring instructions on how to clone and mirror
all data and code used by this external index.