* [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 [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 [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 [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 [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