Re: [PATCH v3 24/27] KVM: s390: arm64: Implement vm/vcpu create destroy.

[sashiko-bot@kernel.org]

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

New issues:
- [High] In `kvm_arch_init_vm`, `kvm->arch.mem_limit` is improperly set to the inclusive hardware limit (`kvm_max_guest_address()`) instead of the VM's requested IPA size (`kvm->arch.guest_phys_size`). This causes `gmap_prepare_memory_region` to incorrectly reject perfectly sized memslots (because it compares an exclusive memslot size against an inclusive limit) and allows userspace to map memslots far beyond the VM's requested address space. These out-of-bounds memslots are then silently ignored during the commit phase by `_dat_walk_gfn_range` (due to `DAT_WALK_IGN_HOLES`), creating a fatal desynchronization between KVM's memslots and the hardware DAT tables.
- [High] `gmap_commit_memory_region` performs a fallible memory allocation (`kvm_s390_new_mmu_cache()`) during the KVM memslot commit phase, which is architecturally required to be non-failable (it returns `void`). If this allocation fails, the function aborts and silently skips updating the gmap DAT tables.

Pre-existing issues:
- [High] `gmap_get_dirty_log()` uses a non-atomic `memset()` to clear the memory slot's `dirty_bitmap` after copying it to userspace via `kvm_get_dirty_log()`. This introduces a severe race condition against concurrent callers of `mark_page_dirty()` (e.g., from interrupt injection in `s390/kvm/s390/interrupt.c` or nested virtualization), which use atomic bit operations (`set_bit_le`).
--

commit 001e0a58ea811dbd19411f46d5babaaf3bbbbc47
Author: Steffen Eiden <seiden@linux.ibm.com>

KVM: s390: arm64: Implement vm/vcpu create destroy.

Implement init and destroy IOCTLS for vcpu and vm.
Implement arch vm IOCTL. Use s390 gmap.

> diff --git a/arch/s390/kvm/arm64/arm.c b/arch/s390/kvm/arm64/arm.c
> index 591e2e874891a..79ce6570fa031 100644
> --- a/arch/s390/kvm/arm64/arm.c
> +++ b/arch/s390/kvm/arm64/arm.c
> @@ -45,6 +48,61 @@ static u64 kvm_max_guest_address(void)
>  	return ALIGN_DOWN(max_addr + 1, 1 << 30) - 1;
>  }

[ ... ]

> +int kvm_arch_init_vm(struct kvm *kvm, unsigned long type)
> +{

[ ... ]

> +	ret = kvm_gmap_init(kvm);
> +	if (ret)
> +		goto out_err;
> +	kvm->arch.mem_limit = kvm_max_guest_address();

[Severity: High]
Does setting kvm->arch.mem_limit to the inclusive hardware limit
kvm_max_guest_address() instead of kvm->arch.guest_phys_size cause
memslot validation issues?

When gmap_prepare_memory_region() validates the memory slots:

arch/s390/kvm/gmap/mmu.c:gmap_prepare_memory_region() {
        ...
        if ((new->base_gfn + new->npages) * PAGE_SIZE > kvm->arch.mem_limit)
                return -EINVAL;
        ...
}

It compares an exclusive size against this inclusive limit. Could this
incorrectly reject perfectly sized memslots?

Additionally, since it uses the maximum host physical capacity instead of
the VM's requested IPA size, could it allow userspace to map memslots beyond
the VM's address space, resulting in a desynchronization between KVM and
the hardware DAT tables?

[ ... ]

> @@ -107,6 +201,52 @@ int kvm_vm_ioctl_irq_line(struct kvm *kvm, struct kvm_irq_level *irq_level,
>  	return 0;
>  }
>  
> +int kvm_vm_ioctl_get_dirty_log(struct kvm *kvm,
> +			       struct kvm_dirty_log *log)
> +{
> +	return gmap_get_dirty_log(kvm, log);
> +}

[Severity: High]
This is a pre-existing issue, but is it safe to use gmap_get_dirty_log()
here given how it clears the dirty bitmap?

In the underlying implementation:

arch/s390/kvm/gmap/mmu.c:gmap_get_dirty_log() {
        ...
        if (is_dirty) {
                n = kvm_dirty_bitmap_bytes(memslot);
                memset(memslot->dirty_bitmap, 0, n);
        }
        ...
}

The memset() is non-atomic. Could this permanently clobber concurrently set
dirty bits from mark_page_dirty(), which uses atomic bit operations? Any
dirty bits set concurrently during or immediately before the memset could
be lost.

[ ... ]

> +void kvm_arch_commit_memory_region(struct kvm *kvm,
> +				   struct kvm_memory_slot *old,
> +				   const struct kvm_memory_slot *new,
> +				   enum kvm_mr_change change)
> +{
> +	gmap_commit_memory_region(kvm, old, new, change);
> +}

[Severity: High]
Is it safe to rely on a fallible allocation inside the void
kvm_arch_commit_memory_region() phase?

Calling gmap_commit_memory_region() here triggers a memory allocation:

arch/s390/kvm/gmap/mmu.c:gmap_commit_memory_region() {
        ...
        mc = kvm_s390_new_mmu_cache();
        if (!mc) {
                rc = -ENOMEM;
                goto out;
        }
        ...
}

Since the commit phase returns void and cannot fail to userspace, if this
allocation fails under memory pressure, it will silently skip updating
the gmap DAT tables. Could this cause KVM's generic memslots to become
permanently desynchronized from the hardware gmap state?

-- 
Sashiko AI review · https://sashiko.dev/#/patchset/20260529155050.2902245-1-seiden@linux.ibm.com?part=24