Re: [PATCH v5 28/31] KVM: s390: arm64: Implement vm/vcpu create destroy.

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

New issues:
- [High] Integer underflow in kvm_max_guest_address() when host memory is less than 1GB bypasses guest physical memory boundaries.
- [High] Incorrect calculation of KVM IPA limit causes VM creation failure for non-power-of-two maximum memory sizes.

Pre-existing issues:
- [High] s390_kvm_mmu_commit_memory_region silently ignores ENOMEM, leaving hardware page tables out of sync with KVM memslots.
--

--- Patch [28]: [PATCH v5 28/31] KVM: s390: arm64: Implement vm/vcpu create destroy. ---
commit 1d8cc2e3c6c2245e41621c3d2e7a8e11e5053070
Author: Steffen Eiden <[email protected]>

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

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

> diff --git a/arch/s390/kvm/arm64/arm.c b/arch/s390/kvm/arm64/arm.c
> index 8e17b7eef8131..3f1bbbf38abec 100644
> --- a/arch/s390/kvm/arm64/arm.c
> +++ b/arch/s390/kvm/arm64/arm.c
[ ... ]
> @@ -53,6 +58,83 @@ static u64 kvm_max_guest_address(void)
>  	return ALIGN_DOWN(max_addr + 1, 1 << 30) - 1;

[Severity: High]
If the host maximum address (sclp.hamax) is less than 1GB - 1, could
max_addr + 1 be strictly less than 1GB?

If so, ALIGN_DOWN(..., 1 << 30) would evaluate to 0, which would underflow to
U64_MAX when subtracting 1.

This U64_MAX value is then used in kvm_arch_vcpu_create() to set the hardware
execution block's memory size limit (msl):

    sae_block->msl = kvm_max_guest_address();

Could this eliminate hardware-enforced limits on guest physical memory,
allowing unprivileged guest VMs to bypass their memory bounds?

>  }
>  
[ ... ]
> @@ -64,6 +146,13 @@ long kvm_arch_dev_ioctl(struct file *filp,
[ ... ]
>  u32 get_kvm_ipa_limit(void)
>  {
>  	return fls64(kvm_max_guest_address() + 1) - 1;

[Severity: High]
Does this formula improperly compute the number of bits required to address
the memory when the maximum address is not a perfect power of two?

For example, if max_addr + 1 is 3GB, fls64(3GB) - 1 evaluates to 31 bits,
but addressing 3GB actually requires 32 bits.

Because ARM64_MIN_PARANGE_BITS is 32, if the calculated limit drops to 31,
kvm_vm_type_ipa_size_shift() will return -EINVAL, which could prevent VM
creation on hosts with memory limits like 3GB.

>  }
>  
[ ... ]
> @@ -103,6 +223,81 @@ static unsigned long system_supported_vcpu_features(void)
[ ... ]
> +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)
> +{
> +	s390_kvm_mmu_commit_memory_region(kvm, old, new, change);

[Severity: High]
This isn't a bug introduced by this patch, but by wiring up
s390_kvm_mmu_commit_memory_region() here, are we exposing a pre-existing
error handling flaw?

In s390_kvm_mmu_commit_memory_region(), if kvm_s390_new_mmu_cache() fails:

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

The function returns without performing the dat_delete_slot() or
dat_create_slot() updates.

Because kvm_arch_commit_memory_region() is called after active memslots are
swapped and cannot return an error, does this leave KVM and the hardware
page tables permanently out of sync?

If a memslot deletion fails to reach the hardware page tables, could the
guest retain access to host memory that has been formally released, leading
to a use-after-free?

> +}
> +

-- 
Sashiko AI review · https://sashiko.dev/#/patchset/[email protected]?part=28