[PATCH 0/4] KVM: arm64: Fix use-after-free in debugfs

From: Oliver Upton <oupton@google.com>
To: kvmarm@lists.cs.columbia.edu
Cc: kvm@vger.kernel.org, Marc Zyngier <maz@kernel.org>,
	James Morse <james.morse@arm.com>,
	 Alexandru Elisei <alexandru.elisei@arm.com>,
	Suzuki K Poulose <suzuki.poulose@arm.com>,
	 linux-arm-kernel@lists.infradead.org,
	Peter Shier <pshier@google.com>,
	 Ricardo Koller <ricarkol@google.com>,
	Reiji Watanabe <reijiw@google.com>,
	 Paolo Bonzini <pbonzini@redhat.com>,
	Sean Christopherson <seanjc@google.com>,
	 Oliver Upton <oupton@google.com>
Subject: [PATCH 0/4] KVM: arm64: Fix use-after-free in debugfs
Date: Sat,  2 Apr 2022 17:40:40 +0000	[thread overview]
Message-ID: <20220402174044.2263418-1-oupton@google.com> (raw)

Funny enough, dirty_log_perf_test on arm64 highlights some issues around
the use of debugfs in KVM. The test leaks a GIC FD across test
iterations, and as such the associated VM is never destroyed.
Nonetheless, the VM FD is reused for the next VM, which collides with
the old debugfs directory.

Where things get off is when the vgic-state debugfs file is created. KVM
does not check if the VM directory exists before creating the file,
which results in the file being added to the root of debugfs when the
aforementioned collision occurs.

Since KVM relies on deleting the VM directory to clean up all associated
files, the errant vgic-state file never gets cleaned up. Poking the file
after the VM is destroyed is a use-after-free :)

Patch 1 takes care of the immediate problem by refusing to create the
file if the VM directory does not exist.

Patch 2 tones down logging around debugfs collisions. As demonstrated by
the selftest, this is most likely to happen for a userspace bug, not
KVM.

The last two patches ensure the GIC FD actually gets closed by the
selftests that use it. Patch 3 is a genuine bug fix since it will create
multiple VMs for a single test run. The arch_timer test also happens to
leak the GIC FD, though it is benign since the test creates a single VM.
Patch 4 gets the arch_timer test to follow the well-behaved pattern.

Applies cleanly to the first KVM pull (tagged kvm-5.18-1), at commit:

  c9b8fecddb5b ("KVM: use kvcalloc for array allocations")

The series is intentionally *not* based on the kvmarm/fixes tree because
there is a small dependency on commit:

  456f89e0928a ("KVM: selftests: aarch64: Skip tests if we can't create a vgic-v3")

which isn't present in the fixes branch.

Tested on an Ampere Altra system in the following combinations:

  - Bad kernel + fixed selftests
  - Fixed kernel + bad selftests

In both cases there was no dmesg spew and no unintended vgic-state file
at the root of debugfs.

Oliver Upton (4):
  KVM: arm64: vgic: Don't assume the VM debugfs directory exists
  KVM: Only log about debugfs directory collision once
  selftests: KVM: Don't leak GIC FD across dirty log test iterations
  selftests: KVM: Free the GIC FD when cleaning up in arch_timer

 arch/arm64/kvm/vgic/vgic-debug.c              |  3 ++
 .../selftests/kvm/aarch64/arch_timer.c        | 15 +++++---
 .../selftests/kvm/dirty_log_perf_test.c       | 34 +++++++++++++++++--
 virt/kvm/kvm_main.c                           |  2 +-
 4 files changed, 46 insertions(+), 8 deletions(-)

-- 
2.35.1.1094.g7c7d902a7c-goog

_______________________________________________
linux-arm-kernel mailing list
linux-arm-kernel@lists.infradead.org
http://lists.infradead.org/mailman/listinfo/linux-arm-kernel