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

[sashiko-bot@kernel.org]

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

New issues:
- [High] Mismatch between kvm->arch.mem_limit and gmap bounds allows an infinite page fault loop.
- [High] kvm_max_guest_address() underflows to U64_MAX when the host's maximum memory address is less than 1GB.
- [Medium] Off-by-one boundary check error due to mismatched mem_limit semantics.

Pre-existing issues:
- [Critical] Lost dirty pages during live migration due to non-atomic memset() in gmap_get_dirty_log().
- [High] Memory allocation in kvm_arch_commit_memory_region allows host Use-After-Free on OOM.
--

commit 07502b29add56a1b469b806719c1e501111b58a8
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 19249ec5c539f..3a4092698fae2 100644
> --- a/arch/s390/kvm/arm64/arm.c
> +++ b/arch/s390/kvm/arm64/arm.c
[ ... ]
> @@ -51,6 +56,82 @@ static u64 kvm_max_guest_address(void)
>  	return ALIGN_DOWN(max_addr + 1, 1 << 30) - 1;

[Severity: High]
Does this ALIGN_DOWN() calculation underflow to U64_MAX if the host's maximum
memory address is less than 1GB? On hosts with low memory configurations
(e.g. sclp.hamax < 1GB), ALIGN_DOWN(max_addr + 1, 1 << 30) evaluates to 0,
causing the subtraction to underflow.

[Severity: Medium]
Will this inclusive boundary cause an off-by-one rejection of valid memory
slots? By subtracting 1 here, kvm_max_guest_address() returns an inclusive
boundary which is later assigned to mem_limit. If a memslot's exclusive end
address perfectly matches the maximum permitted address,
gmap_prepare_memory_region() could reject it:

if ((new->base_gfn + new->npages) * PAGE_SIZE > kvm->arch.mem_limit)
        return -EINVAL;

>  }
>  
> +static int kvm_gmap_init(struct kvm *kvm)
> +{
[ ... ]
> +	kvm->arch.gmap = gmap_new(kvm, gpa_to_gfn(kvm->arch.guest_phys_size));
[ ... ]
> +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]
Can this mismatch between gmap bounds and mem_limit lead to an infinite page
fault loop? The gmap is limited to guest_phys_size (the requested IPA limit),
but mem_limit is set unconditionally to the maximum host-supported address.

If userspace creates a memslot beyond guest_phys_size but below mem_limit,
KVM resolving the PFN and calling gmap_link() would reject the address via
dat_entry_walk(), returning -EAGAIN, causing kvm_s390_faultin_gfn() to retry
endlessly.

[ ... ]
> @@ -113,6 +230,58 @@ 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: Critical]
This is a pre-existing issue, but does gmap_get_dirty_log() lose dirty pages
during live migration? It uses a non-atomic memset() to clear the dirty
bitmap after copying it:

gmap_get_dirty_log() {
    ...
    memset(memslot->dirty_bitmap, 0, n);
    ...
}

If a guest VCPU concurrently faults and marks a page dirty, the atomic bit set
operation in mark_page_dirty() could be blindly overwritten by the memset().

[ ... ]
> +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]
This isn't a bug introduced by this patch, but does the memory allocation in
gmap_commit_memory_region() allow a host use-after-free on OOM? When host
userspace removes a memory slot, KVM calls commit_memory_region(), which
cannot return an error. However, gmap_commit_memory_region() dynamically
allocates memory via kvm_s390_new_mmu_cache():

gmap_commit_memory_region() {
    ...
    mc = kvm_s390_new_mmu_cache();
    if (!mc) {
        rc = -ENOMEM;
        goto out;
    }
    ...
}

If this fails under memory pressure, the function returns early and skips
dat_delete_slot(). The memslot deletion proceeds globally and the host memory
is freed, but the guest retains hardware page table entries pointing to it.

> +}

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