Re: [PATCH v6 30/33] KVM: s390: arm64: Implement vm/vcpu create destroy.

Thank you for your contribution! Sashiko AI review found 2 potential issue(s) to consider:
- [High] Integer underflow in `kvm_max_guest_address()` bypassing KVM's IPA limits and causing Undefined Behavior.
- [High] VM IPA size configuration is ignored for memory boundaries, and mem_limit suffers from an off-by-one error.
--

--- Patch [30]: [PATCH v6 30/33] KVM: s390: arm64: Implement vm/vcpu create destroy. ---
commit ac331802b4f3974b686910d63a6161cfc71cfae4
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.
Implement function (stubs) required by gamp.

> diff --git a/arch/s390/kvm/arm64/arm.c b/arch/s390/kvm/arm64/arm.c
> index 8e17b7eef8131..69d43066f9e1a 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]
Does this underflow if the host is booted with less than 1GB of memory?

If sclp.hamax is less than 1GB, max_addr + 1 is less than 1 << 30.
ALIGN_DOWN would result in 0, and 0 - 1 would underflow to U64_MAX.

[ ... ]

> +static int kvm_vm_type_ipa_size_shift(unsigned long type)
> +{
> +	int phys_shift;
> +
> +	phys_shift = KVM_VM_TYPE_ARM_IPA_SIZE(type);
> +	if (phys_shift) {
> +		if (phys_shift > get_kvm_ipa_limit() ||
> +		    phys_shift < ARM64_MIN_PARANGE_BITS)
> +			return -EINVAL;

[Severity: High]
This seems to cascade from the potential underflow in kvm_max_guest_address().

If get_kvm_ipa_limit() computes fls64(0) - 1, it would return -1 as 0xFFFFFFFF
via u32. Could a malicious userspace then bypass this IPA size check since
phys_shift > get_kvm_ipa_limit() would evaluate to false?

[ ... ]

> +int kvm_arch_init_vm(struct kvm *kvm, unsigned long type)
> +{
> +	char debug_name[32];
> +	int ret;
> +
> +	if (type & ~KVM_VM_TYPE_ARM_IPA_SIZE_MASK)
> +		return -EINVAL;
> +
> +	ret = kvm_vm_type_ipa_size_shift(type);
> +	if (ret < 0)
> +		return ret;
> +	kvm->arch.guest_phys_size = 1UL << ret;

[Severity: High]
If the check in kvm_vm_type_ipa_size_shift() is bypassed, the phys_shift value
(up to 255) would be used in this shift. Would this invoke undefined behavior
since the shift amount could exceed the width of the type?

[ ... ]

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

[Severity: High]
Is it intended that kvm->arch.mem_limit is hardcoded to the global host
maximum instead of the VM's configured IPA size limit
(kvm->arch.guest_phys_size)?

If it uses the host's maximum, could userspace successfully map memory higher
than the configured VM IPA size limit?

Also, kvm_max_guest_address() appears to return an inclusive limit, but when
s390_kvm_mmu_prepare_memory_region() checks this limit:

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

it treats mem_limit as an exclusive boundary. Does this off-by-one prevent
userspace from mapping the final page of the allowed memory region?

[ ... ]

> +int kvm_arch_vcpu_create(struct kvm_vcpu *vcpu)
> +{
> +	struct kvm_sae_block *sae_block = &vcpu->arch.sae_block;
> +	struct kvm_sae_save_area *save_area = &vcpu->arch.save_area;
> +
> +	spin_lock_init(&vcpu->arch.mp_state_lock);
> +
> +	/* Force users to call KVM_ARM_VCPU_INIT */
> +	vcpu_clear_flag(vcpu, VCPU_INITIALIZED);
> +
> +	vcpu->arch.mc = kvm_s390_new_mmu_cache();
> +	if (!vcpu->arch.mc)
> +		return -ENOMEM;
> +
> +	sae_block->hbasce = vcpu->kvm->arch.gmap->asce.val;
> +	sae_block->mso = 0L;
> +	sae_block->msl = kvm_max_guest_address();

[Severity: High]
Does sae_block->msl also need to be scoped to the VM's requested size rather
than the host maximum?

Setting this to the host maximum might remove hardware emulation boundaries if
the guest physical size was configured to be smaller.

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