[RFC,0/2] Use-after-free in kvm_xen_eventfd_update()

Commit Message

Michal Luczaj Dec. 22, 2022, 8:30 p.m. UTC

There is a use-after-free condition due to a race between
kvm_xen_eventfd_deassign() and kvm_xen_eventfd_update():

				mutex_lock(&kvm->lock)
				evtchnfd = idr_find(...)
				mutex_unlock(&kvm->lock)
mutex_lock(&kvm->lock)
evtchnfd = idr_remove(...)
mutex_unlock(&kvm->lock)
synchronize_srcu(&kvm->srcu)
kfree(evtchnfd)
				[evtchnfd is stale now]
				if (evtchnfd->type != data->u.evtchn.type)
					return -EINVAL;
				...

My understanding is that kvm_xen_eventfd_update() forgets to enter SRCU
critical section, and thus synchronize_srcu() in kvm_xen_eventfd_deassign()
does not really synchronize much, which results in prematurly kfree()ing
evtchnfd.

The condition is rather hard to hit (and I sure hope it is not purely
theoretical), but when I throw a mdelay() between the lines, e.g.


and race update/deassign IOCTLs, then KFENCE reports start popping out:

[  299.233021] ==================================================================
[  299.233043] BUG: KFENCE: use-after-free read in kvm_xen_hvm_set_attr+0x7cb/0x930 [kvm]
[  299.233566] Use-after-free read at 0x000000004c7839ea (in kfence-#134):
[  299.233581]  kvm_xen_hvm_set_attr+0x7cb/0x930 [kvm]
[  299.234094]  kvm_arch_vm_ioctl+0xcda/0xee0 [kvm]
[  299.234613]  kvm_vm_ioctl+0x703/0x1320 [kvm]
[  299.235074]  __x64_sys_ioctl+0xb8/0xf0
[  299.235091]  do_syscall_64+0x55/0x80
[  299.235105]  entry_SYSCALL_64_after_hwframe+0x46/0xb0

[  299.235128] kfence-#134: 0x000000008893823b-0x00000000c2c2058a, size=24, cache=kmalloc-32

[  299.235143] allocated by task 941 on cpu 0 at 288.238587s:
[  299.235890]  __kmem_cache_alloc_node+0x357/0x420
[  299.235908]  kmalloc_trace+0x26/0x60
[  299.235921]  kvm_xen_hvm_set_attr+0x119/0x930 [kvm]
[  299.236425]  kvm_arch_vm_ioctl+0xcda/0xee0 [kvm]
[  299.236905]  kvm_vm_ioctl+0x703/0x1320 [kvm]
[  299.237380]  __x64_sys_ioctl+0xb8/0xf0
[  299.237394]  do_syscall_64+0x55/0x80
[  299.237407]  entry_SYSCALL_64_after_hwframe+0x46/0xb0

[  299.237427] freed by task 941 on cpu 0 at 288.576296s:
[  299.238181]  kvm_xen_hvm_set_attr+0x784/0x930 [kvm]
[  299.238666]  kvm_arch_vm_ioctl+0xcda/0xee0 [kvm]
[  299.239094]  kvm_vm_ioctl+0x703/0x1320 [kvm]
[  299.239210]  __x64_sys_ioctl+0xb8/0xf0
[  299.239213]  do_syscall_64+0x55/0x80
[  299.239216]  entry_SYSCALL_64_after_hwframe+0x46/0xb0

[  299.239222] CPU: 3 PID: 944 Comm: a.out Tainted: G    B              6.1.0+ #63
[  299.239227] Hardware name: QEMU Standard PC (i440FX + PIIX, 1996), BIOS Arch Linux 1.16.1-1-1 04/01/2014
[  299.239230] ==================================================================

PATCH 1/2 proposes a fix.
PATCH 2/2 takes the opportunity to further simplify the code a bit.

Michal Luczaj (2):
  KVM: x86/xen: Fix use-after-free in kvm_xen_eventfd_update()
  KVM: x86/xen: Simplify eventfd IOCTLs

 arch/x86/kvm/xen.c | 27 +++++++++++++++------------
 1 file changed, 15 insertions(+), 12 deletions(-)

Patch

diff --git a/arch/x86/kvm/xen.c b/arch/x86/kvm/xen.c
index d7af40240248..2b3495517c99 100644
--- a/arch/x86/kvm/xen.c
+++ b/arch/x86/kvm/xen.c
@@ -14,6 +14,7 @@ 
 #include <linux/eventfd.h>
 #include <linux/kvm_host.h>
 #include <linux/sched/stat.h>
+#include <linux/delay.h>

 #include <trace/events/kvm.h>
 #include <xen/interface/xen.h>
@@ -1836,6 +1837,8 @@  static int kvm_xen_eventfd_update(struct kvm *kvm,
        if (!evtchnfd)
                return -ENOENT;

+       mdelay(1);
+
        /* For an UPDATE, nothing may change except the priority/vcpu */
        if (evtchnfd->type != data->u.evtchn.type)
                return -EINVAL;