[PATCH] KVM: selftests: verify toggling KVM_MEM_GUEST_MEMFD on an existing slot is rejected

[PATCH] KVM: selftests: verify toggling KVM_MEM_GUEST_MEMFD on an existing slot is rejected

[Valery Borovsky]

set_memory_region_test covers creating a private memslot and rejecting
overlapping private regions, but nothing exercises a FLAGS_ONLY update
that toggles KVM_MEM_GUEST_MEMFD on a slot that already exists.

Add test_toggle_private_memory_region() to close that gap. It asserts
both directions are rejected with EINVAL:

  1. clearing KVM_MEM_GUEST_MEMFD on a private slot;
  2. setting KVM_MEM_GUEST_MEMFD on a slot created without it.

Both are enforced by the flags mask in kvm_set_memory_region():

  (mem->flags ^ old->flags) & (KVM_MEM_READONLY | KVM_MEM_GUEST_MEMFD)

which gained KVM_MEM_GUEST_MEMFD in commit 9935df5333aa ("KVM: Disallow
toggling KVM_MEM_GUEST_MEMFD on an existing memslot"). Before that
commit, direction 1 freed the old slot without calling kvm_gmem_unbind(),
so closing the guest_memfd afterwards wrote to freed memory; the fix
commit records the resulting KASAN slab-use-after-free in
kvm_gmem_release().

The test runs only when KVM_X86_SW_PROTECTED_VM is available, alongside
the existing private-memslot tests.

Signed-off-by: Valery Borovsky <vebohr@gmail.com>
---
Built and run on x86_64 against a 7.0 kernel; the new subtest passes.

 .../selftests/kvm/set_memory_region_test.c    | 45 +++++++++++++++++++
 1 file changed, 45 insertions(+)

diff --git a/tools/testing/selftests/kvm/set_memory_region_test.c b/tools/testing/selftests/kvm/set_memory_region_test.c
index a152ab65c657..8fb2e7a96f6c 100644
--- a/tools/testing/selftests/kvm/set_memory_region_test.c
+++ b/tools/testing/selftests/kvm/set_memory_region_test.c
@@ -500,6 +500,50 @@ static void test_add_private_memory_region(void)
 	kvm_vm_free(vm);
 }
 
+static void test_toggle_private_memory_region(void)
+{
+	struct kvm_vm *vm;
+	int memfd, r;
+
+	pr_info("Testing that toggling KVM_MEM_GUEST_MEMFD on existing slot is rejected\n");
+
+	vm = vm_create_barebones_type(KVM_X86_SW_PROTECTED_VM);
+	memfd = vm_create_guest_memfd(vm, MEM_REGION_SIZE, 0);
+
+	/* Create a private slot. */
+	vm_set_user_memory_region2(vm, MEM_REGION_SLOT, KVM_MEM_GUEST_MEMFD,
+				   MEM_REGION_GPA, MEM_REGION_SIZE,
+				   0, memfd, 0);
+
+	/*
+	 * Attempt a FLAGS_ONLY update that clears KVM_MEM_GUEST_MEMFD.
+	 * Must fail with EINVAL. Before commit 9935df5333aa this caused
+	 * a slab-use-after-free in kvm_gmem_release() because the old slot
+	 * was freed without calling kvm_gmem_unbind().
+	 */
+	r = __vm_set_user_memory_region2(vm, MEM_REGION_SLOT, 0,
+					 MEM_REGION_GPA, MEM_REGION_SIZE,
+					 0, memfd, 0);
+	TEST_ASSERT(r == -1 && errno == EINVAL,
+		    "Clearing KVM_MEM_GUEST_MEMFD should have failed with EINVAL, got r=%d errno=%d", r, errno);
+
+	/*
+	 * Symmetrically, attempting to set KVM_MEM_GUEST_MEMFD on a slot
+	 * that was created without it must also be rejected.
+	 */
+	vm_set_user_memory_region(vm, MEM_REGION_SLOT + 1, 0,
+				  MEM_REGION_GPA * 2, MEM_REGION_SIZE, NULL);
+	r = __vm_set_user_memory_region2(vm, MEM_REGION_SLOT + 1,
+					 KVM_MEM_GUEST_MEMFD,
+					 MEM_REGION_GPA * 2, MEM_REGION_SIZE,
+					 0, memfd, 0);
+	TEST_ASSERT(r == -1 && errno == EINVAL,
+		    "Setting KVM_MEM_GUEST_MEMFD should have failed with EINVAL, got r=%d errno=%d", r, errno);
+
+	close(memfd);
+	kvm_vm_free(vm);
+}
+
 static void test_add_overlapping_private_memory_regions(void)
 {
 	struct kvm_vm *vm;
@@ -645,6 +689,7 @@ int main(int argc, char *argv[])
 	    (kvm_check_cap(KVM_CAP_VM_TYPES) & BIT(KVM_X86_SW_PROTECTED_VM))) {
 		test_add_private_memory_region();
 		test_add_overlapping_private_memory_regions();
+		test_toggle_private_memory_region();
 	} else {
 		pr_info("Skipping tests for KVM_MEM_GUEST_MEMFD memory regions\n");
 	}
-- 
2.53.0

Re: [PATCH] KVM: selftests: verify toggling KVM_MEM_GUEST_MEMFD on an existing slot is rejected

[sashiko-bot]

Thank you for your contribution! Sashiko AI review found 2 potential issue(s) to consider:
- [Medium] The commit message incorrectly claims that both directions of toggling KVM_MEM_GUEST_MEMFD are enforced by the XOR flags mask.
- [Low] The second test case reuses a memfd and offset that are already bound, which could lead to shadowed errors if KVM validation ordering changes.
--

--- Patch [1]: [PATCH] KVM: selftests: verify toggling KVM_MEM_GUEST_MEMFD on an existing slot is rejected ---
commit bf15eec5cbe6f11d42dcd195f6bac07b5502de7d
Author: Valery Borovsky <vebohr@gmail.com>

KVM: selftests: verify toggling KVM_MEM_GUEST_MEMFD on an existing slot is rejected

This commit adds a new test case, test_toggle_private_memory_region(), to
the set_memory_region_test. It verifies that KVM rejects attempts to both
clear KVM_MEM_GUEST_MEMFD on a private slot and set it on a slot created
without it.

> Add test_toggle_private_memory_region() to close that gap. It asserts
> both directions are rejected with EINVAL:
> 
>   1. clearing KVM_MEM_GUEST_MEMFD on a private slot;
>   2. setting KVM_MEM_GUEST_MEMFD on a slot created without it.
> 
> Both are enforced by the flags mask in kvm_set_memory_region():
> 
>   (mem->flags ^ old->flags) & (KVM_MEM_READONLY | KVM_MEM_GUEST_MEMFD)

[Severity: Medium]
Is this explanation entirely accurate for direction 2 (setting the flag)?

Looking at kvm_set_memory_region() in virt/kvm/kvm_main.c, setting
KVM_MEM_GUEST_MEMFD on an existing slot seems to be explicitly rejected
earlier in the function:

if (mem->flags & KVM_MEM_GUEST_MEMFD)
        return -EINVAL;

Could the commit message be updated to clarify that direction 2 is caught by
this early check before the XOR flags mask is evaluated?

> diff --git a/tools/testing/selftests/kvm/set_memory_region_test.c b/tools/testing/selftests/kvm/set_memory_region_test.c
> index a152ab65c6577..8fb2e7a96f6cd 100644
> --- a/tools/testing/selftests/kvm/set_memory_region_test.c
> +++ b/tools/testing/selftests/kvm/set_memory_region_test.c

[ ... ]

> @@ -500,6 +500,50 @@ static void test_add_private_memory_region(void)
>  	kvm_vm_free(vm);
>  }
>  
> +static void test_toggle_private_memory_region(void)
> +{

[ ... ]

> +	/*
> +	 * Symmetrically, attempting to set KVM_MEM_GUEST_MEMFD on a slot
> +	 * that was created without it must also be rejected.
> +	 */
> +	vm_set_user_memory_region(vm, MEM_REGION_SLOT + 1, 0,
> +				  MEM_REGION_GPA * 2, MEM_REGION_SIZE, NULL);
> +	r = __vm_set_user_memory_region2(vm, MEM_REGION_SLOT + 1,
> +					 KVM_MEM_GUEST_MEMFD,
> +					 MEM_REGION_GPA * 2, MEM_REGION_SIZE,
> +					 0, memfd, 0);

[Severity: Low]
Does reusing the same memfd and offset 0 make this test unnecessarily brittle?

This call uses a memfd and offset that were already successfully bound to
MEM_REGION_SLOT earlier in the test function. If KVM were to ever reorder
its internal validation and evaluate kvm_gmem_bind() before rejecting the
immutable flag toggle, wouldn't this return -EEXIST due to the already-bound
range, causing a false test failure instead of the expected -EINVAL?

Would it be safer to use a fresh memfd or a non-overlapping offset to
strictly isolate the flag toggle validation?

> +	TEST_ASSERT(r == -1 && errno == EINVAL,
> +		    "Setting KVM_MEM_GUEST_MEMFD should have failed with EINVAL, got r=%d errno=%d", r, errno);
> +
> +	close(memfd);
> +	kvm_vm_free(vm);
> +}

-- 
Sashiko AI review · https://sashiko.dev/#/patchset/20260805055830.740175-1-vebohr@gmail.com?part=1