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

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

[Oliver Upton]

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

[PATCH 1/4] KVM: arm64: vgic: Don't assume the VM debugfs directory exists

[Oliver Upton]

Unfortunately, there is no guarantee that KVM was able to instantiate a
debugfs directory for a particular VM. To that end, KVM shouldn't even
attempt to create new debugfs files in this case. If the specified
parent dentry is NULL, debugfs_create_file() will instantiate files at
the root of debugfs.

Since it is possible to create the vgic-state file outside of a VM
directory, the file is not cleaned up when a VM is destroyed.
Nonetheless, the corresponding struct kvm is freed when the VM is
destroyed.

Plug the use-after-free by plainly refusing to create vgic-state when
KVM fails to create a VM debugfs dir.

Cc: stable@kernel.org
Fixes: 929f45e32499 ("kvm: no need to check return value of debugfs_create functions")
Signed-off-by: Oliver Upton <oupton@google.com>
---
 arch/arm64/kvm/vgic/vgic-debug.c | 3 +++
 1 file changed, 3 insertions(+)

diff --git a/arch/arm64/kvm/vgic/vgic-debug.c b/arch/arm64/kvm/vgic/vgic-debug.c
index f38c40a76251..cf1364a6fabc 100644
--- a/arch/arm64/kvm/vgic/vgic-debug.c
+++ b/arch/arm64/kvm/vgic/vgic-debug.c
@@ -271,6 +271,9 @@ DEFINE_SEQ_ATTRIBUTE(vgic_debug);
 
 void vgic_debug_init(struct kvm *kvm)
 {
+	if (!kvm->debugfs_dentry)
+		return;
+
 	debugfs_create_file("vgic-state", 0444, kvm->debugfs_dentry, kvm,
 			    &vgic_debug_fops);
 }
-- 
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

Re: [PATCH 1/4] KVM: arm64: vgic: Don't assume the VM debugfs directory exists

[Oliver Upton]

On Sat, Apr 02, 2022 at 05:40:41PM +0000, Oliver Upton wrote:
> Unfortunately, there is no guarantee that KVM was able to instantiate a
> debugfs directory for a particular VM. To that end, KVM shouldn't even
> attempt to create new debugfs files in this case. If the specified
> parent dentry is NULL, debugfs_create_file() will instantiate files at
> the root of debugfs.
> 
> Since it is possible to create the vgic-state file outside of a VM
> directory, the file is not cleaned up when a VM is destroyed.
> Nonetheless, the corresponding struct kvm is freed when the VM is
> destroyed.
> 
> Plug the use-after-free by plainly refusing to create vgic-state when
> KVM fails to create a VM debugfs dir.
> 
> Cc: stable@kernel.org
> Fixes: 929f45e32499 ("kvm: no need to check return value of debugfs_create functions")
> Signed-off-by: Oliver Upton <oupton@google.com>

Thinking about this a bit more...

The game of whack-a-mole for other files that possibly have the same bug
could probably be avoided if kvm->debugfs_dentry is initialized to
PTR_ERR(-ENOENT) by default. That way there's no special error handling
that needs to be done in KVM as any attempt to create a new file will
bail.

Going to test and send out a v2 most likely, just want to make sure no
other use of debugfs in KVM will flip out from the change.

--
Thanks,
Oliver

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

[PATCH 2/4] KVM: Only log about debugfs directory collision once

[Oliver Upton]

In all likelihood, a debugfs directory name collision is the result of a
userspace bug. If userspace closes the VM fd without releasing all
references to said VM then the debugfs directory is never cleaned.

Even a ratelimited print statement can fill up dmesg, making it
particularly annoying for the person debugging what exactly went wrong.
Furthermore, a userspace that wants to be a nuisance could clog up the
logs by deliberately holding a VM reference after closing the VM fd.

Dial back logging to print at most once, given that userspace is most
likely to blame. Leave the statement in place for the small chance that
KVM actually got it wrong.

Cc: stable@kernel.org
Fixes: 85cd39af14f4 ("KVM: Do not leak memory for duplicate debugfs directories")
Signed-off-by: Oliver Upton <oupton@google.com>
---
 virt/kvm/kvm_main.c | 2 +-
 1 file changed, 1 insertion(+), 1 deletion(-)

diff --git a/virt/kvm/kvm_main.c b/virt/kvm/kvm_main.c
index 69c318fdff61..38b30bd60f34 100644
--- a/virt/kvm/kvm_main.c
+++ b/virt/kvm/kvm_main.c
@@ -959,7 +959,7 @@ static int kvm_create_vm_debugfs(struct kvm *kvm, int fd)
 	mutex_lock(&kvm_debugfs_lock);
 	dent = debugfs_lookup(dir_name, kvm_debugfs_dir);
 	if (dent) {
-		pr_warn_ratelimited("KVM: debugfs: duplicate directory %s\n", dir_name);
+		pr_warn_once("KVM: debugfs: duplicate directory %s\n", dir_name);
 		dput(dent);
 		mutex_unlock(&kvm_debugfs_lock);
 		return 0;
-- 
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

Re: [PATCH 2/4] KVM: Only log about debugfs directory collision once

[Sean Christopherson]

On Sat, Apr 02, 2022, Oliver Upton wrote:
> In all likelihood, a debugfs directory name collision is the result of a
> userspace bug. If userspace closes the VM fd without releasing all
> references to said VM then the debugfs directory is never cleaned.
> 
> Even a ratelimited print statement can fill up dmesg, making it
> particularly annoying for the person debugging what exactly went wrong.
> Furthermore, a userspace that wants to be a nuisance could clog up the
> logs by deliberately holding a VM reference after closing the VM fd.
> 
> Dial back logging to print at most once, given that userspace is most
> likely to blame. Leave the statement in place for the small chance that
> KVM actually got it wrong.
> 
> Cc: stable@kernel.org
> Fixes: 85cd39af14f4 ("KVM: Do not leak memory for duplicate debugfs directories")

I don't think this warrants Cc: stable@, the whole point of ratelimiting printk is
to guard against this sort of thing.  If a ratelimited printk can bring down the
kernel and/or logging infrastructure, then the kernel is misconfigured for the
environment.

> Signed-off-by: Oliver Upton <oupton@google.com>
> ---
>  virt/kvm/kvm_main.c | 2 +-
>  1 file changed, 1 insertion(+), 1 deletion(-)
> 
> diff --git a/virt/kvm/kvm_main.c b/virt/kvm/kvm_main.c
> index 69c318fdff61..38b30bd60f34 100644
> --- a/virt/kvm/kvm_main.c
> +++ b/virt/kvm/kvm_main.c
> @@ -959,7 +959,7 @@ static int kvm_create_vm_debugfs(struct kvm *kvm, int fd)
>  	mutex_lock(&kvm_debugfs_lock);
>  	dent = debugfs_lookup(dir_name, kvm_debugfs_dir);
>  	if (dent) {
> -		pr_warn_ratelimited("KVM: debugfs: duplicate directory %s\n", dir_name);
> +		pr_warn_once("KVM: debugfs: duplicate directory %s\n", dir_name);

I don't see how printing once is going to be usefull for a human debugger.  If we
want to get rid of the ratelimited print, why not purge it entirely?

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

Re: [PATCH 2/4] KVM: Only log about debugfs directory collision once

[Oliver Upton]

Hi Sean,

On Mon, Apr 04, 2022 at 05:33:29PM +0000, Sean Christopherson wrote:
> On Sat, Apr 02, 2022, Oliver Upton wrote:
> > In all likelihood, a debugfs directory name collision is the result of a
> > userspace bug. If userspace closes the VM fd without releasing all
> > references to said VM then the debugfs directory is never cleaned.
> > 
> > Even a ratelimited print statement can fill up dmesg, making it
> > particularly annoying for the person debugging what exactly went wrong.
> > Furthermore, a userspace that wants to be a nuisance could clog up the
> > logs by deliberately holding a VM reference after closing the VM fd.
> > 
> > Dial back logging to print at most once, given that userspace is most
> > likely to blame. Leave the statement in place for the small chance that
> > KVM actually got it wrong.
> > 
> > Cc: stable@kernel.org
> > Fixes: 85cd39af14f4 ("KVM: Do not leak memory for duplicate debugfs directories")
> 
> I don't think this warrants Cc: stable@, the whole point of ratelimiting printk is
> to guard against this sort of thing.  If a ratelimited printk can bring down the
> kernel and/or logging infrastructure, then the kernel is misconfigured for the
> environment.

Good point.

> > Signed-off-by: Oliver Upton <oupton@google.com>
> > ---
> >  virt/kvm/kvm_main.c | 2 +-
> >  1 file changed, 1 insertion(+), 1 deletion(-)
> > 
> > diff --git a/virt/kvm/kvm_main.c b/virt/kvm/kvm_main.c
> > index 69c318fdff61..38b30bd60f34 100644
> > --- a/virt/kvm/kvm_main.c
> > +++ b/virt/kvm/kvm_main.c
> > @@ -959,7 +959,7 @@ static int kvm_create_vm_debugfs(struct kvm *kvm, int fd)
> >  	mutex_lock(&kvm_debugfs_lock);
> >  	dent = debugfs_lookup(dir_name, kvm_debugfs_dir);
> >  	if (dent) {
> > -		pr_warn_ratelimited("KVM: debugfs: duplicate directory %s\n", dir_name);
> > +		pr_warn_once("KVM: debugfs: duplicate directory %s\n", dir_name);
> 
> I don't see how printing once is going to be usefull for a human debugger.  If we
> want to get rid of the ratelimited print, why not purge it entirely?

I'd really like to drop it altogether. I've actually looked at several
instances of this printk firing internally, and all of it had to do with
some leak in userspace.

I'll pull this patch out of the series for v2 and maybe just propose we
drop it altogether.

--
Thanks,
Oliver

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

[PATCH 3/4] selftests: KVM: Don't leak GIC FD across dirty log test iterations

[Oliver Upton]

dirty_log_perf_test instantiates a VGICv3 for the guest (if supported by
hardware) to reduce the overhead of guest exits. However, the test does
not actually close the GIC fd when cleaning up the VM between test
iterations, meaning that the VM is never actually destroyed in the
kernel.

While this is generally a bad idea, the bug was detected from the kernel
spewing about duplicate debugfs entries as subsequent VMs happen to
reuse the same FD even though the debugfs directory is still present.

Abstract away the notion of setup/cleanup of the GIC FD from the test
by creating arch-specific helpers for test setup/cleanup. Close the GIC
FD on VM cleanup and do nothing for the other architectures.

Fixes: c340f7899af6 ("KVM: selftests: Add vgic initialization for dirty log perf test for ARM")
Cc: Jing Zhang <jingzhangos@google.com>
Signed-off-by: Oliver Upton <oupton@google.com>
---
 .../selftests/kvm/dirty_log_perf_test.c       | 34 +++++++++++++++++--
 1 file changed, 31 insertions(+), 3 deletions(-)

diff --git a/tools/testing/selftests/kvm/dirty_log_perf_test.c b/tools/testing/selftests/kvm/dirty_log_perf_test.c
index c9d9e513ca04..7b47ae4f952e 100644
--- a/tools/testing/selftests/kvm/dirty_log_perf_test.c
+++ b/tools/testing/selftests/kvm/dirty_log_perf_test.c
@@ -18,11 +18,40 @@
 #include "test_util.h"
 #include "perf_test_util.h"
 #include "guest_modes.h"
+
 #ifdef __aarch64__
 #include "aarch64/vgic.h"
 
 #define GICD_BASE_GPA			0x8000000ULL
 #define GICR_BASE_GPA			0x80A0000ULL
+
+static int gic_fd;
+
+static void arch_setup_vm(struct kvm_vm *vm, unsigned int nr_vcpus)
+{
+	/*
+	 * The test can still run even if hardware does not support GICv3, as it
+	 * is only an optimization to reduce guest exits.
+	 */
+	gic_fd = vgic_v3_setup(vm, nr_vcpus, 64, GICD_BASE_GPA, GICR_BASE_GPA);
+}
+
+static void arch_cleanup_vm(struct kvm_vm *vm)
+{
+	if (gic_fd > 0)
+		close(gic_fd);
+}
+
+#else /* __aarch64__ */
+
+static void arch_setup_vm(struct kvm_vm *vm, unsigned int nr_vcpus)
+{
+}
+
+static void arch_cleanup_vm(struct kvm_vm *vm)
+{
+}
+
 #endif
 
 /* How many host loops to run by default (one KVM_GET_DIRTY_LOG for each loop)*/
@@ -206,9 +235,7 @@ static void run_test(enum vm_guest_mode mode, void *arg)
 		vm_enable_cap(vm, &cap);
 	}
 
-#ifdef __aarch64__
-	vgic_v3_setup(vm, nr_vcpus, 64, GICD_BASE_GPA, GICR_BASE_GPA);
-#endif
+	arch_setup_vm(vm, nr_vcpus);
 
 	/* Start the iterations */
 	iteration = 0;
@@ -302,6 +329,7 @@ static void run_test(enum vm_guest_mode mode, void *arg)
 	}
 
 	free_bitmaps(bitmaps, p->slots);
+	arch_cleanup_vm(vm);
 	perf_test_destroy_vm(vm);
 }
 
-- 
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

Re: [PATCH 3/4] selftests: KVM: Don't leak GIC FD across dirty log test iterations

[Jing Zhang]

On Sat, Apr 2, 2022 at 10:40 AM Oliver Upton <oupton@google.com> wrote:
>
> dirty_log_perf_test instantiates a VGICv3 for the guest (if supported by
> hardware) to reduce the overhead of guest exits. However, the test does
> not actually close the GIC fd when cleaning up the VM between test
> iterations, meaning that the VM is never actually destroyed in the
> kernel.
>
> While this is generally a bad idea, the bug was detected from the kernel
> spewing about duplicate debugfs entries as subsequent VMs happen to
> reuse the same FD even though the debugfs directory is still present.
>
> Abstract away the notion of setup/cleanup of the GIC FD from the test
> by creating arch-specific helpers for test setup/cleanup. Close the GIC
> FD on VM cleanup and do nothing for the other architectures.
>
> Fixes: c340f7899af6 ("KVM: selftests: Add vgic initialization for dirty log perf test for ARM")
> Cc: Jing Zhang <jingzhangos@google.com>
> Signed-off-by: Oliver Upton <oupton@google.com>
> ---
>  .../selftests/kvm/dirty_log_perf_test.c       | 34 +++++++++++++++++--
>  1 file changed, 31 insertions(+), 3 deletions(-)
>
> diff --git a/tools/testing/selftests/kvm/dirty_log_perf_test.c b/tools/testing/selftests/kvm/dirty_log_perf_test.c
> index c9d9e513ca04..7b47ae4f952e 100644
> --- a/tools/testing/selftests/kvm/dirty_log_perf_test.c
> +++ b/tools/testing/selftests/kvm/dirty_log_perf_test.c
> @@ -18,11 +18,40 @@
>  #include "test_util.h"
>  #include "perf_test_util.h"
>  #include "guest_modes.h"
> +
>  #ifdef __aarch64__
>  #include "aarch64/vgic.h"
>
>  #define GICD_BASE_GPA                  0x8000000ULL
>  #define GICR_BASE_GPA                  0x80A0000ULL
> +
> +static int gic_fd;
> +
> +static void arch_setup_vm(struct kvm_vm *vm, unsigned int nr_vcpus)
> +{
> +       /*
> +        * The test can still run even if hardware does not support GICv3, as it
> +        * is only an optimization to reduce guest exits.
> +        */
> +       gic_fd = vgic_v3_setup(vm, nr_vcpus, 64, GICD_BASE_GPA, GICR_BASE_GPA);
> +}
> +
> +static void arch_cleanup_vm(struct kvm_vm *vm)
> +{
> +       if (gic_fd > 0)
> +               close(gic_fd);
> +}
> +
> +#else /* __aarch64__ */
> +
> +static void arch_setup_vm(struct kvm_vm *vm, unsigned int nr_vcpus)
> +{
> +}
> +
> +static void arch_cleanup_vm(struct kvm_vm *vm)
> +{
> +}
> +
>  #endif
>
>  /* How many host loops to run by default (one KVM_GET_DIRTY_LOG for each loop)*/
> @@ -206,9 +235,7 @@ static void run_test(enum vm_guest_mode mode, void *arg)
>                 vm_enable_cap(vm, &cap);
>         }
>
> -#ifdef __aarch64__
> -       vgic_v3_setup(vm, nr_vcpus, 64, GICD_BASE_GPA, GICR_BASE_GPA);
> -#endif
> +       arch_setup_vm(vm, nr_vcpus);
>
>         /* Start the iterations */
>         iteration = 0;
> @@ -302,6 +329,7 @@ static void run_test(enum vm_guest_mode mode, void *arg)
>         }
>
>         free_bitmaps(bitmaps, p->slots);
> +       arch_cleanup_vm(vm);
>         perf_test_destroy_vm(vm);
>  }
>
> --
> 2.35.1.1094.g7c7d902a7c-goog
>
Reviewed-by: Jing Zhang <jingzhangos@google.com>

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

[PATCH 4/4] selftests: KVM: Free the GIC FD when cleaning up in arch_timer

[Oliver Upton]

In order to correctly destroy a VM, all references to the VM must be
freed. The arch_timer selftest creates a VGIC for the guest, which
itself holds a reference to the VM.

Close the GIC FD when cleaning up a VM.

Signed-off-by: Oliver Upton <oupton@google.com>
---
 tools/testing/selftests/kvm/aarch64/arch_timer.c | 15 +++++++++++----
 1 file changed, 11 insertions(+), 4 deletions(-)

diff --git a/tools/testing/selftests/kvm/aarch64/arch_timer.c b/tools/testing/selftests/kvm/aarch64/arch_timer.c
index b08d30bf71c5..3b940a101bc0 100644
--- a/tools/testing/selftests/kvm/aarch64/arch_timer.c
+++ b/tools/testing/selftests/kvm/aarch64/arch_timer.c
@@ -362,11 +362,12 @@ static void test_init_timer_irq(struct kvm_vm *vm)
 	pr_debug("ptimer_irq: %d; vtimer_irq: %d\n", ptimer_irq, vtimer_irq);
 }
 
+static int gic_fd;
+
 static struct kvm_vm *test_vm_create(void)
 {
 	struct kvm_vm *vm;
 	unsigned int i;
-	int ret;
 	int nr_vcpus = test_args.nr_vcpus;
 
 	vm = vm_create_default_with_vcpus(nr_vcpus, 0, 0, guest_code, NULL);
@@ -383,8 +384,8 @@ static struct kvm_vm *test_vm_create(void)
 
 	ucall_init(vm, NULL);
 	test_init_timer_irq(vm);
-	ret = vgic_v3_setup(vm, nr_vcpus, 64, GICD_BASE_GPA, GICR_BASE_GPA);
-	if (ret < 0) {
+	gic_fd = vgic_v3_setup(vm, nr_vcpus, 64, GICD_BASE_GPA, GICR_BASE_GPA);
+	if (gic_fd < 0) {
 		print_skip("Failed to create vgic-v3");
 		exit(KSFT_SKIP);
 	}
@@ -395,6 +396,12 @@ static struct kvm_vm *test_vm_create(void)
 	return vm;
 }
 
+static void test_vm_cleanup(struct kvm_vm *vm)
+{
+	close(gic_fd);
+	kvm_vm_free(vm);
+}
+
 static void test_print_help(char *name)
 {
 	pr_info("Usage: %s [-h] [-n nr_vcpus] [-i iterations] [-p timer_period_ms]\n",
@@ -478,7 +485,7 @@ int main(int argc, char *argv[])
 
 	vm = test_vm_create();
 	test_run(vm);
-	kvm_vm_free(vm);
+	test_vm_cleanup(vm);
 
 	return 0;
 }
-- 
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