kvm.vger.kernel.org archive mirror
 help / color / mirror / Atom feed
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 v2 0/3] KVM: Fix use-after-free in debugfs
Date: Mon,  4 Apr 2022 18:21:16 +0000	[thread overview]
Message-ID: <20220404182119.3561025-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 shuts the door on any mistaken debugfs file creations by
initializing kvm->debugfs_dentry with ERR_PTR() instead of NULL.

The last two patches ensure the GIC FD actually gets closed by the
selftests that use it. Patch 2 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 3 gets the arch_timer test to follow the well-behaved pattern.

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

  c15e0ae42c8e ("KVM: x86: fix sending PV IPI")

Tested on an Ampere Altra system in the following combinations:

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

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

v1: https://lore.kernel.org/kvm/20220402174044.2263418-1-oupton@google.com/

v1 -> v2:
 - Initialize kvm->debugfs_dentry to ERR_PTR(-ENOENT) to prevent the
   creation of all VM debug files, not just vgic-state.
 - Leave logging as-is for now. Consider dropping altogether in a later
   patch (Sean)
 - Collect R-b from Jing

Oliver Upton (3):
  KVM: Don't create VM debugfs files outside of the VM directory
  selftests: KVM: Don't leak GIC FD across dirty log test iterations
  selftests: KVM: Free the GIC FD when cleaning up in arch_timer

 .../selftests/kvm/aarch64/arch_timer.c        | 15 +++++---
 .../selftests/kvm/dirty_log_perf_test.c       | 34 +++++++++++++++++--
 virt/kvm/kvm_main.c                           | 10 ++++--
 3 files changed, 50 insertions(+), 9 deletions(-)

-- 
2.35.1.1094.g7c7d902a7c-goog


             reply	other threads:[~2022-04-04 21:23 UTC|newest]

Thread overview: 6+ messages / expand[flat|nested]  mbox.gz  Atom feed  top
2022-04-04 18:21 Oliver Upton [this message]
2022-04-04 18:21 ` [PATCH v2 1/3] KVM: Don't create VM debugfs files outside of the VM directory Oliver Upton
2022-04-06  7:10   ` Marc Zyngier
2022-04-06 17:59     ` Oliver Upton
2022-04-04 18:21 ` [PATCH v2 2/3] selftests: KVM: Don't leak GIC FD across dirty log test iterations Oliver Upton
2022-04-04 18:21 ` [PATCH v2 3/3] selftests: KVM: Free the GIC FD when cleaning up in arch_timer Oliver Upton

Reply instructions:

You may reply publicly to this message via plain-text email
using any one of the following methods:

* Save the following mbox file, import it into your mail client,
  and reply-to-all from there: mbox

  Avoid top-posting and favor interleaved quoting:
  https://en.wikipedia.org/wiki/Posting_style#Interleaved_style

* Reply using the --to, --cc, and --in-reply-to
  switches of git-send-email(1):

  git send-email \
    --in-reply-to=20220404182119.3561025-1-oupton@google.com \
    --to=oupton@google.com \
    --cc=alexandru.elisei@arm.com \
    --cc=james.morse@arm.com \
    --cc=kvm@vger.kernel.org \
    --cc=kvmarm@lists.cs.columbia.edu \
    --cc=linux-arm-kernel@lists.infradead.org \
    --cc=maz@kernel.org \
    --cc=pbonzini@redhat.com \
    --cc=pshier@google.com \
    --cc=reijiw@google.com \
    --cc=ricarkol@google.com \
    --cc=seanjc@google.com \
    --cc=suzuki.poulose@arm.com \
    /path/to/YOUR_REPLY

  https://kernel.org/pub/software/scm/git/docs/git-send-email.html

* If your mail client supports setting the In-Reply-To header
  via mailto: links, try the mailto: link
Be sure your reply has a Subject: header at the top and a blank line before the message body.
This is a public inbox, see mirroring instructions
for how to clone and mirror all data and code used for this inbox;
as well as URLs for NNTP newsgroup(s).