[PATCH] KVM: Mask off the address space ID in kvm_set_internal_memslot() check

[PATCH] KVM: Mask off the address space ID in kvm_set_internal_memslot() check

[Zeng Chi]

From: Zeng Chi <zengchi@kylinos.cn>

kvm_set_internal_memslot() is supposed to reject memslot IDs that belong
to userspace, i.e. IDs below KVM_USER_MEM_SLOTS, but it compares the raw
"slot" field, which packs both the address space ID and the memslot ID
(as_id << 16 | id).  For any address space other than 0, the packed value
is always >= 65536, which is larger than KVM_USER_MEM_SLOTS, and so the
sanity check is a nop regardless of the actual memslot ID.

E.g. x86 sets internal memslots for every address space, including SMM,
so the check would silently accept a userspace memslot ID for the SMM
address space while rejecting the same ID for the normal address space.

Extract the memslot ID the same way kvm_vm_ioctl_set_memory_region() and
kvm_set_memory_region() do, so that the check covers all address spaces.
No in-tree caller trips the check; this only makes the sanity check
effective for all address spaces.

Fixes: 156bffdb2b49 ("KVM: Add a dedicated API for setting KVM-internal memslots")
Signed-off-by: Zeng Chi <zengchi@kylinos.cn>
---
 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 65eb26a0520d..e8d0e360ea26 100644
--- a/virt/kvm/kvm_main.c
+++ b/virt/kvm/kvm_main.c
@@ -2128,7 +2128,7 @@ static int kvm_set_memory_region(struct kvm *kvm,
 int kvm_set_internal_memslot(struct kvm *kvm,
 			     const struct kvm_userspace_memory_region2 *mem)
 {
-	if (WARN_ON_ONCE(mem->slot < KVM_USER_MEM_SLOTS))
+	if (WARN_ON_ONCE((u16)mem->slot < KVM_USER_MEM_SLOTS))
 		return -EINVAL;
 
 	if (WARN_ON_ONCE(mem->flags))
-- 
2.25.1


No virus found
		Checked by Hillstone Network AntiVirus

Re: [PATCH] KVM: Mask off the address space ID in kvm_set_internal_memslot() check

[sashiko-bot]

Thank you for your contribution! Sashiko AI review found 1 potential issue(s) to consider:

Pre-existing issues:
- [High] `__x86_set_memory_region()` leaks user virtual memory allocated via `vm_mmap` and leaves the VM in a corrupted partial state on error paths when `kvm_set_internal_memslot()` fails.
--

commit 1d364f757ed17cf4e343c19a31d2b4aeb3b66e5f
Author: Zeng Chi <zengchi@kylinos.cn>

KVM: Mask off the address space ID in kvm_set_internal_memslot() check

This commit fixes a sanity check in kvm_set_internal_memslot to correctly
isolate and compare just the memslot ID against KVM_USER_MEM_SLOTS. By
masking off the address space ID, the check is now effective across all
address spaces rather than silently passing for any address space other
than 0.

> diff --git a/virt/kvm/kvm_main.c b/virt/kvm/kvm_main.c
> index 65eb26a0520d8..e8d0e360ea26b 100644
> --- a/virt/kvm/kvm_main.c
> +++ b/virt/kvm/kvm_main.c
> @@ -2128,7 +2128,7 @@ static int kvm_set_memory_region(struct kvm *kvm,
>  int kvm_set_internal_memslot(struct kvm *kvm,
>  			     const struct kvm_userspace_memory_region2 *mem)
>  {
> -	if (WARN_ON_ONCE(mem->slot < KVM_USER_MEM_SLOTS))
> +	if (WARN_ON_ONCE((u16)mem->slot < KVM_USER_MEM_SLOTS))
>  		return -EINVAL;
>  
>  	if (WARN_ON_ONCE(mem->flags))

[Severity: High]
This is a pre-existing issue, but I have a question about the error handling
in arch/x86/kvm/x86.c:__x86_set_memory_region() when
kvm_set_internal_memslot() fails.

When size > 0, the function allocates a VMA mapping via vm_mmap() and
iterates over all address space IDs:

        for (i = 0; i < kvm_arch_nr_memslot_as_ids(kvm); i++) {
                struct kvm_userspace_memory_region2 m;
                ...
                r = kvm_set_internal_memslot(kvm, &m);
                if (r < 0)
                        return ERR_PTR_USR(r);
        }

If kvm_set_internal_memslot() fails for any address space, it returns early.

Does this early return skip the vm_munmap() cleanup and leak the VMA mapping
allocated by vm_mmap() in the host process?

Additionally, if the loop fails on a later address space, earlier successfully
created memslots are left behind. During VM destruction, kvm_arch_destroy_vm()
calls __x86_set_memory_region() with size = 0 to free these internal memslots.
Will this deletion loop fail with -EINVAL when it attempts to delete the
non-existent slot for the previously failed address space? Does this bypass
the final vm_munmap() step and leak the VMA mapping?

-- 
Sashiko AI review · https://sashiko.dev/#/patchset/20260827112200.866476-1-zeng_chi911@163.com?part=1