[PATCH 0/7] KVM: Grab KVM references for stats fds

[PATCH 0/7] KVM: Grab KVM references for stats fds

[Sean Christopherson]

Grab a reference to the VM when handing a userspace stats fds for VMs and
vCPUs to ensure the stats files don't outlive the VM and its vCPUs, and add
a regression testcase in selftests.

Sean Christopherson (7):
  KVM: Grab a reference to KVM for VM and vCPU stats file descriptors
  KVM: selftests: Use pread() to read binary stats header
  KVM: selftests: Clean up stats fd in common stats_test() helper
  KVM: selftests: Explicitly free vcpus array in binary stats test
  KVM: selftests: Verify userspace can create "redundant" binary stats
    files
  KVM: selftests: Verify stats fd can be dup()'d and read
  KVM: selftests: Verify stats fd is usable after VM fd has been closed

 .../selftests/kvm/include/kvm_util_base.h     |  6 +-
 .../selftests/kvm/kvm_binary_stats_test.c     | 72 ++++++++++++-------
 virt/kvm/kvm_main.c                           | 24 +++++++
 3 files changed, 75 insertions(+), 27 deletions(-)


base-commit: 255006adb3da71bb75c334453786df781b415f54
-- 
2.41.0.255.g8b1d071c50-goog

[PATCH 1/7] KVM: Grab a reference to KVM for VM and vCPU stats file descriptors

[Sean Christopherson]

Grab a reference to KVM prior to installing VM and vCPU stats file
descriptors to ensure the underlying VM and vCPU objects are not freed
until the last reference to any and all stats fds are dropped.

Note, the stats paths manually invoke fd_install() and so don't need to
grab a reference before creating the file.

Fixes: ce55c049459c ("KVM: stats: Support binary stats retrieval for a VCPU")
Fixes: fcfe1baeddbf ("KVM: stats: Support binary stats retrieval for a VM")
Reported-by: Zheng Zhang <zheng.zhang@email.ucr.edu>
Closes: https://lore.kernel.org/all/CAC_GQSr3xzZaeZt85k_RCBd5kfiOve8qXo7a81Cq53LuVQ5r=Q@mail.gmail.com
Cc: stable@vger.kernel.org
Cc: Kees Cook <keescook@chromium.org>
Signed-off-by: Sean Christopherson <seanjc@google.com>
---
 virt/kvm/kvm_main.c | 24 ++++++++++++++++++++++++
 1 file changed, 24 insertions(+)

diff --git a/virt/kvm/kvm_main.c b/virt/kvm/kvm_main.c
index b838c8f71349..312a8d9184fe 100644
--- a/virt/kvm/kvm_main.c
+++ b/virt/kvm/kvm_main.c
@@ -4032,8 +4032,17 @@ static ssize_t kvm_vcpu_stats_read(struct file *file, char __user *user_buffer,
 			sizeof(vcpu->stat), user_buffer, size, offset);
 }
 
+static int kvm_vcpu_stats_release(struct inode *inode, struct file *file)
+{
+	struct kvm_vcpu *vcpu = file->private_data;
+
+	kvm_put_kvm(vcpu->kvm);
+	return 0;
+}
+
 static const struct file_operations kvm_vcpu_stats_fops = {
 	.read = kvm_vcpu_stats_read,
+	.release = kvm_vcpu_stats_release,
 	.llseek = noop_llseek,
 };
 
@@ -4054,6 +4063,9 @@ static int kvm_vcpu_ioctl_get_stats_fd(struct kvm_vcpu *vcpu)
 		put_unused_fd(fd);
 		return PTR_ERR(file);
 	}
+
+	kvm_get_kvm(vcpu->kvm);
+
 	file->f_mode |= FMODE_PREAD;
 	fd_install(fd, file);
 
@@ -4698,8 +4710,17 @@ static ssize_t kvm_vm_stats_read(struct file *file, char __user *user_buffer,
 				sizeof(kvm->stat), user_buffer, size, offset);
 }
 
+static int kvm_vm_stats_release(struct inode *inode, struct file *file)
+{
+	struct kvm *kvm = file->private_data;
+
+	kvm_put_kvm(kvm);
+	return 0;
+}
+
 static const struct file_operations kvm_vm_stats_fops = {
 	.read = kvm_vm_stats_read,
+	.release = kvm_vm_stats_release,
 	.llseek = noop_llseek,
 };
 
@@ -4718,6 +4739,9 @@ static int kvm_vm_ioctl_get_stats_fd(struct kvm *kvm)
 		put_unused_fd(fd);
 		return PTR_ERR(file);
 	}
+
+	kvm_get_kvm(kvm);
+
 	file->f_mode |= FMODE_PREAD;
 	fd_install(fd, file);
 
-- 
2.41.0.255.g8b1d071c50-goog

Re: [PATCH 1/7] KVM: Grab a reference to KVM for VM and vCPU stats file descriptors

[Kees Cook]

On Tue, Jul 11, 2023 at 04:01:25PM -0700, Sean Christopherson wrote:
> Grab a reference to KVM prior to installing VM and vCPU stats file
> descriptors to ensure the underlying VM and vCPU objects are not freed
> until the last reference to any and all stats fds are dropped.
> 
> Note, the stats paths manually invoke fd_install() and so don't need to
> grab a reference before creating the file.
> 
> Fixes: ce55c049459c ("KVM: stats: Support binary stats retrieval for a VCPU")
> Fixes: fcfe1baeddbf ("KVM: stats: Support binary stats retrieval for a VM")
> Reported-by: Zheng Zhang <zheng.zhang@email.ucr.edu>
> Closes: https://lore.kernel.org/all/CAC_GQSr3xzZaeZt85k_RCBd5kfiOve8qXo7a81Cq53LuVQ5r=Q@mail.gmail.com
> Cc: stable@vger.kernel.org
> Cc: Kees Cook <keescook@chromium.org>
> Signed-off-by: Sean Christopherson <seanjc@google.com>

Thanks for preparing this! Looks like the common get/put code pattern,
so I can review this patch, unlike the rest of the series. :)

Reviewed-by: Kees Cook <keescook@chromium.org>

-- 
Kees Cook

[PATCH 2/7] KVM: selftests: Use pread() to read binary stats header

[Sean Christopherson]

Use pread() with an explicit offset when reading the header and the header
name for a binary stats fd so that the common helper and the binary stats
test don't subtly rely on the file effectively being untouched, e.g. to
allow multiple reads of the header, name, etc.

Signed-off-by: Sean Christopherson <seanjc@google.com>
---
 tools/testing/selftests/kvm/include/kvm_util_base.h | 6 ++++--
 tools/testing/selftests/kvm/kvm_binary_stats_test.c | 6 ++++--
 2 files changed, 8 insertions(+), 4 deletions(-)

diff --git a/tools/testing/selftests/kvm/include/kvm_util_base.h b/tools/testing/selftests/kvm/include/kvm_util_base.h
index 07732a157ccd..eb1ff597bcca 100644
--- a/tools/testing/selftests/kvm/include/kvm_util_base.h
+++ b/tools/testing/selftests/kvm/include/kvm_util_base.h
@@ -362,8 +362,10 @@ static inline void read_stats_header(int stats_fd, struct kvm_stats_header *head
 {
 	ssize_t ret;
 
-	ret = read(stats_fd, header, sizeof(*header));
-	TEST_ASSERT(ret == sizeof(*header), "Read stats header");
+	ret = pread(stats_fd, header, sizeof(*header), 0);
+	TEST_ASSERT(ret == sizeof(*header),
+		    "Failed to read '%lu' header bytes, ret = '%ld'",
+		    sizeof(*header), ret);
 }
 
 struct kvm_stats_desc *read_stats_descriptors(int stats_fd,
diff --git a/tools/testing/selftests/kvm/kvm_binary_stats_test.c b/tools/testing/selftests/kvm/kvm_binary_stats_test.c
index a7001e29dc06..eae99d0e8377 100644
--- a/tools/testing/selftests/kvm/kvm_binary_stats_test.c
+++ b/tools/testing/selftests/kvm/kvm_binary_stats_test.c
@@ -43,8 +43,10 @@ static void stats_test(int stats_fd)
 	id = malloc(header.name_size);
 	TEST_ASSERT(id, "Allocate memory for id string");
 
-	ret = read(stats_fd, id, header.name_size);
-	TEST_ASSERT(ret == header.name_size, "Read id string");
+	ret = pread(stats_fd, id, header.name_size, sizeof(header));
+	TEST_ASSERT(ret == header.name_size,
+		    "Expected header size '%u', read '%lu' bytes",
+		    header.name_size, ret);
 
 	/* Check id string, that should start with "kvm" */
 	TEST_ASSERT(!strncmp(id, "kvm", 3) && strlen(id) < header.name_size,
-- 
2.41.0.255.g8b1d071c50-goog

[PATCH 3/7] KVM: selftests: Clean up stats fd in common stats_test() helper

[Sean Christopherson]

Move the stats fd cleanup code into stats_test() and drop the
superfluous vm_stats_test() and vcpu_stats_test() helpers in order to
decouple creation of the stats file from consuming/testing the file
(deduping code is a bonus).  This will make it easier to test various
edge cases related to stats, e.g. that userspace can dup() a stats fd,
that userspace can have multiple stats files for a singleVM/vCPU, etc.

Signed-off-by: Sean Christopherson <seanjc@google.com>
---
 .../selftests/kvm/kvm_binary_stats_test.c     | 22 ++++---------------
 1 file changed, 4 insertions(+), 18 deletions(-)

diff --git a/tools/testing/selftests/kvm/kvm_binary_stats_test.c b/tools/testing/selftests/kvm/kvm_binary_stats_test.c
index eae99d0e8377..f02663711c90 100644
--- a/tools/testing/selftests/kvm/kvm_binary_stats_test.c
+++ b/tools/testing/selftests/kvm/kvm_binary_stats_test.c
@@ -167,23 +167,7 @@ static void stats_test(int stats_fd)
 	free(stats_data);
 	free(stats_desc);
 	free(id);
-}
 
-
-static void vm_stats_test(struct kvm_vm *vm)
-{
-	int stats_fd = vm_get_stats_fd(vm);
-
-	stats_test(stats_fd);
-	close(stats_fd);
-	TEST_ASSERT(fcntl(stats_fd, F_GETFD) == -1, "Stats fd not freed");
-}
-
-static void vcpu_stats_test(struct kvm_vcpu *vcpu)
-{
-	int stats_fd = vcpu_get_stats_fd(vcpu);
-
-	stats_test(stats_fd);
 	close(stats_fd);
 	TEST_ASSERT(fcntl(stats_fd, F_GETFD) == -1, "Stats fd not freed");
 }
@@ -241,9 +225,11 @@ int main(int argc, char *argv[])
 
 	/* Check stats read for every VM and VCPU */
 	for (i = 0; i < max_vm; ++i) {
-		vm_stats_test(vms[i]);
+		stats_test(vm_get_stats_fd(vms[i]));
+
 		for (j = 0; j < max_vcpu; ++j)
-			vcpu_stats_test(vcpus[i * max_vcpu + j]);
+			stats_test(vcpu_get_stats_fd(vcpus[i * max_vcpu + j]));
+
 		ksft_test_result_pass("vm%i\n", i);
 	}
 
-- 
2.41.0.255.g8b1d071c50-goog

[PATCH 4/7] KVM: selftests: Explicitly free vcpus array in binary stats test

[Sean Christopherson]

Explicitly free the all-encompassing vcpus array in the binary stats test
so that the test is consistent with respect to freeing all dynamically
allocated resources (versus letting them be freed on exit).

Signed-off-by: Sean Christopherson <seanjc@google.com>
---
 tools/testing/selftests/kvm/kvm_binary_stats_test.c | 1 +
 1 file changed, 1 insertion(+)

diff --git a/tools/testing/selftests/kvm/kvm_binary_stats_test.c b/tools/testing/selftests/kvm/kvm_binary_stats_test.c
index f02663711c90..874fa5092551 100644
--- a/tools/testing/selftests/kvm/kvm_binary_stats_test.c
+++ b/tools/testing/selftests/kvm/kvm_binary_stats_test.c
@@ -236,6 +236,7 @@ int main(int argc, char *argv[])
 	for (i = 0; i < max_vm; ++i)
 		kvm_vm_free(vms[i]);
 	free(vms);
+	free(vcpus);
 
 	ksft_finished();	/* Print results and exit() accordingly */
 }
-- 
2.41.0.255.g8b1d071c50-goog

[PATCH 5/7] KVM: selftests: Verify userspace can create "redundant" binary stats files

[Sean Christopherson]

Verify that KVM doesn't artificially limit KVM_GET_STATS_FD to a single
file per VM/vCPU.  There's no known use case for getting multiple stats
fds, but it should work, and more importantly creating multiple files will
make it easier to test that KVM correct manages VM refcounts for stats
files.

Signed-off-by: Sean Christopherson <seanjc@google.com>
---
 .../selftests/kvm/kvm_binary_stats_test.c     | 25 +++++++++++++++++--
 1 file changed, 23 insertions(+), 2 deletions(-)

diff --git a/tools/testing/selftests/kvm/kvm_binary_stats_test.c b/tools/testing/selftests/kvm/kvm_binary_stats_test.c
index 874fa5092551..653f10d8fb7c 100644
--- a/tools/testing/selftests/kvm/kvm_binary_stats_test.c
+++ b/tools/testing/selftests/kvm/kvm_binary_stats_test.c
@@ -185,6 +185,7 @@ static void stats_test(int stats_fd)
 
 int main(int argc, char *argv[])
 {
+	int vm_stats_fds, *vcpu_stats_fds;
 	int i, j;
 	struct kvm_vcpu **vcpus;
 	struct kvm_vm **vms;
@@ -217,18 +218,37 @@ int main(int argc, char *argv[])
 	vcpus = malloc(sizeof(struct kvm_vcpu *) * max_vm * max_vcpu);
 	TEST_ASSERT(vcpus, "Allocate memory for storing vCPU pointers");
 
+	/*
+	 * Not per-VM as the array is populated, used, and invalidated within a
+	 * single for-loop iteration.
+	 */
+	vcpu_stats_fds = calloc(max_vm, sizeof(*vcpu_stats_fds));
+	TEST_ASSERT(vcpu_stats_fds, "Allocate memory for VM stats fds");
+
 	for (i = 0; i < max_vm; ++i) {
 		vms[i] = vm_create_barebones();
 		for (j = 0; j < max_vcpu; ++j)
 			vcpus[i * max_vcpu + j] = __vm_vcpu_add(vms[i], j);
 	}
 
-	/* Check stats read for every VM and VCPU */
+	/*
+	 * Check stats read for every VM and vCPU, with a variety of testcases.
+	 * Note, stats_test() closes the passed in stats fd.
+	 */
 	for (i = 0; i < max_vm; ++i) {
+		vm_stats_fds = vm_get_stats_fd(vms[i]);
+
+		/* Verify userspace can instantiate multiple stats files. */
 		stats_test(vm_get_stats_fd(vms[i]));
 
-		for (j = 0; j < max_vcpu; ++j)
+		for (j = 0; j < max_vcpu; ++j) {
+			vcpu_stats_fds[j] = vcpu_get_stats_fd(vcpus[i * max_vcpu + j]);
 			stats_test(vcpu_get_stats_fd(vcpus[i * max_vcpu + j]));
+		}
+
+		stats_test(vm_stats_fds);
+		for (j = 0; j < max_vcpu; ++j)
+			stats_test(vcpu_stats_fds[j]);
 
 		ksft_test_result_pass("vm%i\n", i);
 	}
@@ -237,6 +257,7 @@ int main(int argc, char *argv[])
 		kvm_vm_free(vms[i]);
 	free(vms);
 	free(vcpus);
+	free(vcpu_stats_fds);
 
 	ksft_finished();	/* Print results and exit() accordingly */
 }
-- 
2.41.0.255.g8b1d071c50-goog

[PATCH 6/7] KVM: selftests: Verify stats fd can be dup()'d and read

[Sean Christopherson]

Expand the binary stats test to verify that a stats fd can be dup()'d
and read, to (very) roughly simulate userspace passing around the file.
Adding the dup() test is primarily an intermediate step towards verifying
that userspace can read VM/vCPU stats before _and_ after userspace closes
its copy of the VM fd; the dup() test itself is only mildly interesting.

Signed-off-by: Sean Christopherson <seanjc@google.com>
---
 tools/testing/selftests/kvm/kvm_binary_stats_test.c | 8 +++++++-
 1 file changed, 7 insertions(+), 1 deletion(-)

diff --git a/tools/testing/selftests/kvm/kvm_binary_stats_test.c b/tools/testing/selftests/kvm/kvm_binary_stats_test.c
index 653f10d8fb7c..5317e27b77d0 100644
--- a/tools/testing/selftests/kvm/kvm_binary_stats_test.c
+++ b/tools/testing/selftests/kvm/kvm_binary_stats_test.c
@@ -232,17 +232,23 @@ int main(int argc, char *argv[])
 	}
 
 	/*
-	 * Check stats read for every VM and vCPU, with a variety of testcases.
+	 * Check stats read for every VM and vCPU, with a variety of flavors.
 	 * Note, stats_test() closes the passed in stats fd.
 	 */
 	for (i = 0; i < max_vm; ++i) {
+		/*
+		 * Verify that creating multiple userspace references to a
+		 * single stats file works and doesn't cause explosions.
+		 */
 		vm_stats_fds = vm_get_stats_fd(vms[i]);
+		stats_test(dup(vm_stats_fds));
 
 		/* Verify userspace can instantiate multiple stats files. */
 		stats_test(vm_get_stats_fd(vms[i]));
 
 		for (j = 0; j < max_vcpu; ++j) {
 			vcpu_stats_fds[j] = vcpu_get_stats_fd(vcpus[i * max_vcpu + j]);
+			stats_test(dup(vcpu_stats_fds[j]));
 			stats_test(vcpu_get_stats_fd(vcpus[i * max_vcpu + j]));
 		}
 
-- 
2.41.0.255.g8b1d071c50-goog

[PATCH 7/7] KVM: selftests: Verify stats fd is usable after VM fd has been closed

[Sean Christopherson]

Verify that VM and vCPU binary stats files are usable even after userspace
has put its last direct reference to the VM.  This is a regression test
for a UAF bug where KVM didn't gift the stats files a reference to the VM.

Signed-off-by: Sean Christopherson <seanjc@google.com>
---
 tools/testing/selftests/kvm/kvm_binary_stats_test.c | 10 ++++++++--
 1 file changed, 8 insertions(+), 2 deletions(-)

diff --git a/tools/testing/selftests/kvm/kvm_binary_stats_test.c b/tools/testing/selftests/kvm/kvm_binary_stats_test.c
index 5317e27b77d0..698c1cfa3111 100644
--- a/tools/testing/selftests/kvm/kvm_binary_stats_test.c
+++ b/tools/testing/selftests/kvm/kvm_binary_stats_test.c
@@ -252,6 +252,14 @@ int main(int argc, char *argv[])
 			stats_test(vcpu_get_stats_fd(vcpus[i * max_vcpu + j]));
 		}
 
+		/*
+		 * Close the VM fd and redo the stats tests.  KVM should gift a
+		 * reference (to the VM) to each stats fd, i.e. stats should
+		 * still be accessible even after userspace has put its last
+		 * _direct_ reference to the VM.
+		 */
+		kvm_vm_free(vms[i]);
+
 		stats_test(vm_stats_fds);
 		for (j = 0; j < max_vcpu; ++j)
 			stats_test(vcpu_stats_fds[j]);
@@ -259,8 +267,6 @@ int main(int argc, char *argv[])
 		ksft_test_result_pass("vm%i\n", i);
 	}
 
-	for (i = 0; i < max_vm; ++i)
-		kvm_vm_free(vms[i]);
 	free(vms);
 	free(vcpus);
 	free(vcpu_stats_fds);
-- 
2.41.0.255.g8b1d071c50-goog

Re: [PATCH 0/7] KVM: Grab KVM references for stats fds

[Sean Christopherson]

On Tue, 11 Jul 2023 16:01:24 -0700, Sean Christopherson wrote:
> Grab a reference to the VM when handing a userspace stats fds for VMs and
> vCPUs to ensure the stats files don't outlive the VM and its vCPUs, and add
> a regression testcase in selftests.
> 
> Sean Christopherson (7):
>   KVM: Grab a reference to KVM for VM and vCPU stats file descriptors
>   KVM: selftests: Use pread() to read binary stats header
>   KVM: selftests: Clean up stats fd in common stats_test() helper
>   KVM: selftests: Explicitly free vcpus array in binary stats test
>   KVM: selftests: Verify userspace can create "redundant" binary stats
>     files
>   KVM: selftests: Verify stats fd can be dup()'d and read
>   KVM: selftests: Verify stats fd is usable after VM fd has been closed
> 
> [...]

Applied to kvm-x86 fixes, thanks!

[1/7] KVM: Grab a reference to KVM for VM and vCPU stats file descriptors
      https://github.com/kvm-x86/linux/commit/0d6d1727a21e
[2/7] KVM: selftests: Use pread() to read binary stats header
      https://github.com/kvm-x86/linux/commit/ba6ed5058b1e
[3/7] KVM: selftests: Clean up stats fd in common stats_test() helper
      https://github.com/kvm-x86/linux/commit/34ffae5d4294
[4/7] KVM: selftests: Explicitly free vcpus array in binary stats test
      https://github.com/kvm-x86/linux/commit/0dec04897a5c
[5/7] KVM: selftests: Verify userspace can create "redundant" binary stats files
      https://github.com/kvm-x86/linux/commit/518f3fde1f28
[6/7] KVM: selftests: Verify stats fd can be dup()'d and read
      https://github.com/kvm-x86/linux/commit/a4b51af2c290
[7/7] KVM: selftests: Verify stats fd is usable after VM fd has been closed
      https://github.com/kvm-x86/linux/commit/00b6b7e96803

--
https://github.com/kvm-x86/linux/tree/next
https://github.com/kvm-x86/linux/tree/fixes

Re: [PATCH 0/7] KVM: Grab KVM references for stats fds

[Paolo Bonzini]

Queued, thanks.

Paolo