[PATCH] KVM: VMX: Postpone IPIv setup after successful vCPU creation

[Dmytro Maluka]

vCPU creation in kvm_vm_ioctl_create_vcpu() may fail after
kvm_arch_vcpu_create() -> vmx_vcpu_create() already succeeded. In such
case kvm_vm_ioctl_create_vcpu() destroys the newly created vCPU in the
failure path. However, that leaves a side effect: the IPIv pid_table
entry remains configured with this vCPU's pi_desc address. As a result,
when another vCPU sends an IPI to the APIC ID of this failed-to-create
vCPU, it will cause HW to write to this (freed!) pi_desc memory. [*]

An easy fix would be to clear the pid_table entry in vmx_vcpu_free().
However that would be still problematic, for the following reason:
userspace may try to create a vCPU with the same vcpu_id as an existing
one; vmx_vcpu_create() will succeed, and only after that
kvm_vm_ioctl_create_vcpu() will check for the duplicate vcpu_id and
fail with -EEXIST and then free the vCPU in the failure path. So in this
failure path, vmx_vcpu_free() would clear the pid_table entry for that
already existing good vCPU, i.e. effectively disable IPIv for that vCPU.

So instead fix the issue by moving the pid_table entry setup from
.vcpu_create() to the newly introduced .vcpu_postcreate() callback
called at the end of kvm_vm_ioctl_create_vcpu(), when we are sure that
the vCPU creation succeeded.

[*] Although, since this memory is freed into the kvm_vcpu_cache kmem
    cache which is only used for allocating kvm_vcpus, _maybe_ this
    memory will only be reused for pi_desc of another vCPU, not for
    anything else. So _maybe_ this will only result in delivering the
    IPI to a wrong vCPU (possibly of another VM) in the worst case, not
    in a random corruption of kernel memory.

Fixes: d588bb9be1da ("KVM: VMX: enable IPI virtualization")
Signed-off-by: Dmytro Maluka <dmaluka@xxxxxxxxxxxx>
---
 arch/x86/include/asm/kvm-x86-ops.h |  1 +
 arch/x86/include/asm/kvm_host.h    |  1 +
 arch/x86/kvm/vmx/main.c            |  9 +++++++++
 arch/x86/kvm/vmx/vmx.c             | 11 +++++++----
 arch/x86/kvm/vmx/x86_ops.h         |  1 +
 arch/x86/kvm/x86.c                 |  1 +
 6 files changed, 20 insertions(+), 4 deletions(-)

diff --git a/arch/x86/include/asm/kvm-x86-ops.h b/arch/x86/include/asm/kvm-x86-ops.h
index 83dc5086138b..aecbe9c54004 100644
--- a/arch/x86/include/asm/kvm-x86-ops.h
+++ b/arch/x86/include/asm/kvm-x86-ops.h
@@ -25,6 +25,7 @@ KVM_X86_OP_OPTIONAL(vm_destroy)
 KVM_X86_OP_OPTIONAL(vm_pre_destroy)
 KVM_X86_OP_OPTIONAL_RET0(vcpu_precreate)
 KVM_X86_OP(vcpu_create)
+KVM_X86_OP_OPTIONAL(vcpu_postcreate)
 KVM_X86_OP(vcpu_free)
 KVM_X86_OP(vcpu_reset)
 KVM_X86_OP(prepare_switch_to_guest)
diff --git a/arch/x86/include/asm/kvm_host.h b/arch/x86/include/asm/kvm_host.h
index 5f6c1ce9673b..3ed9d8fc3b96 100644
--- a/arch/x86/include/asm/kvm_host.h
+++ b/arch/x86/include/asm/kvm_host.h
@@ -1803,6 +1803,7 @@ struct kvm_x86_ops {
 	/* Create, but do not attach this VCPU */
 	int (*vcpu_precreate)(struct kvm *kvm);
 	int (*vcpu_create)(struct kvm_vcpu *vcpu);
+	void (*vcpu_postcreate)(struct kvm_vcpu *vcpu);
 	void (*vcpu_free)(struct kvm_vcpu *vcpu);
 	void (*vcpu_reset)(struct kvm_vcpu *vcpu, bool init_event);
 
diff --git a/arch/x86/kvm/vmx/main.c b/arch/x86/kvm/vmx/main.c
index 83d9921277ea..bdf5ba3155f6 100644
--- a/arch/x86/kvm/vmx/main.c
+++ b/arch/x86/kvm/vmx/main.c
@@ -78,6 +78,14 @@ static int vt_vcpu_create(struct kvm_vcpu *vcpu)
 	return vmx_vcpu_create(vcpu);
 }
 
+static void vt_vcpu_postcreate(struct kvm_vcpu *vcpu)
+{
+	if (is_td_vcpu(vcpu))
+		return;
+
+	vmx_vcpu_postcreate(vcpu);
+}
+
 static void vt_vcpu_free(struct kvm_vcpu *vcpu)
 {
 	if (is_td_vcpu(vcpu)) {
@@ -898,6 +906,7 @@ struct kvm_x86_ops vt_x86_ops __initdata = {
 
 	.vcpu_precreate = vt_op(vcpu_precreate),
 	.vcpu_create = vt_op(vcpu_create),
+	.vcpu_postcreate = vt_op(vcpu_postcreate),
 	.vcpu_free = vt_op(vcpu_free),
 	.vcpu_reset = vt_op(vcpu_reset),
 
diff --git a/arch/x86/kvm/vmx/vmx.c b/arch/x86/kvm/vmx/vmx.c
index cc75feec05da..ae6cef9c13e0 100644
--- a/arch/x86/kvm/vmx/vmx.c
+++ b/arch/x86/kvm/vmx/vmx.c
@@ -7752,10 +7752,6 @@ int vmx_vcpu_create(struct kvm_vcpu *vcpu)
 		vmx->ve_info = page_to_virt(page);
 	}
 
-	if (vmx_can_use_ipiv(vcpu))
-		WRITE_ONCE(to_kvm_vmx(vcpu->kvm)->pid_table[vcpu->vcpu_id],
-			   __pa(&vmx->vt.pi_desc) | PID_TABLE_ENTRY_VALID);
-
 	return 0;
 
 free_vmcs:
@@ -7767,6 +7763,13 @@ int vmx_vcpu_create(struct kvm_vcpu *vcpu)
 	return err;
 }
 
+void vmx_vcpu_postcreate(struct kvm_vcpu *vcpu)
+{
+	if (vmx_can_use_ipiv(vcpu))
+		WRITE_ONCE(to_kvm_vmx(vcpu->kvm)->pid_table[vcpu->vcpu_id],
+			   __pa(&to_vmx(vcpu)->vt.pi_desc) | PID_TABLE_ENTRY_VALID);
+}
+
 #define L1TF_MSG_SMT "L1TF CPU bug present and SMT on, data leak possible. See CVE-2018-3646 and https://www.kernel.org/doc/html/latest/admin-guide/hw-vuln/l1tf.html for details.\n"
 #define L1TF_MSG_L1D "L1TF CPU bug present and virtualization mitigation disabled, data leak possible. See CVE-2018-3646 and https://www.kernel.org/doc/html/latest/admin-guide/hw-vuln/l1tf.html for details.\n"
 
diff --git a/arch/x86/kvm/vmx/x86_ops.h b/arch/x86/kvm/vmx/x86_ops.h
index 409858074246..defc2c3a37c8 100644
--- a/arch/x86/kvm/vmx/x86_ops.h
+++ b/arch/x86/kvm/vmx/x86_ops.h
@@ -21,6 +21,7 @@ int vmx_vm_init(struct kvm *kvm);
 void vmx_vm_destroy(struct kvm *kvm);
 int vmx_vcpu_precreate(struct kvm *kvm);
 int vmx_vcpu_create(struct kvm_vcpu *vcpu);
+void vmx_vcpu_postcreate(struct kvm_vcpu *vcpu);
 int vmx_vcpu_pre_run(struct kvm_vcpu *vcpu);
 fastpath_t vmx_vcpu_run(struct kvm_vcpu *vcpu, u64 run_flags);
 void vmx_vcpu_free(struct kvm_vcpu *vcpu);
diff --git a/arch/x86/kvm/x86.c b/arch/x86/kvm/x86.c
index afcac1042947..c9f512a30fec 100644
--- a/arch/x86/kvm/x86.c
+++ b/arch/x86/kvm/x86.c
@@ -12873,6 +12873,7 @@ void kvm_arch_vcpu_postcreate(struct kvm_vcpu *vcpu)
 		return;
 	vcpu_load(vcpu);
 	kvm_synchronize_tsc(vcpu, NULL);
+	kvm_x86_call(vcpu_postcreate)(vcpu);
 	vcpu_put(vcpu);
 
 	/* poll control enabled by default */
-- 
2.55.0.141.g00534a21ce-goog