Re: [PATCH v2 8/8] KVM: s390: Prevent memslots outside the ASCE range

[sashiko-bot@kernel.org]

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

New issues:
- [High] Dropping `kvm->slots_lock` before updating the ASCE limit creates a TOCTOU race and data race condition that allows memslots to bypass the new limit and risk reading torn values.
- [High] The exact user-provided memory limit is discarded and new memslots are checked against a coarse, rounded-up hardware ASCE boundary, allowing userspace to bypass the exact limit.

Pre-existing issues:
- [High] Setting a sub-page limit (e.g., 4095 bytes) silently removes the memory limit by expanding the VM address space to the maximum 16 Exabytes.
--

commit afeca87ebcc558baee29b01c63a93364ae7ae27f
Author: Claudio Imbrenda <imbrenda@linux.ibm.com>

KVM: s390: Prevent memslots outside the ASCE range

With KVM_S390_VM_MEM_LIMIT_SIZE, userspace can set the highest address
allowed for the VM. This patch prevents creation of memslots over the
maximum address, and prevents the maximum address from being reduced
below the end of existing memslots.

> diff --git a/arch/s390/kvm/kvm-s390.c b/arch/s390/kvm/kvm-s390.c
> index e09960c2e6ed1..1796a7c645a9d 100644
> --- a/arch/s390/kvm/kvm-s390.c
> +++ b/arch/s390/kvm/kvm-s390.c
> @@ -1014,12 +1019,27 @@ static int kvm_s390_set_mem_control(struct kvm *kvm, struct kvm_device_attr *att
>  		if (!new_limit)
>  			return -EINVAL;

[Severity: High]
This is a pre-existing issue, but does this check fail to reject sub-page
size limits that truncate to 0 GFNs?

If a sub-page limit (e.g., 4095) is passed, it passes this zero check but
truncates to 0 when converted via gpa_to_gfn(new_limit) later in the function.

When gmap_set_limit() is called with a 0 limit, gmap_limit_to_type(0)
returns TABLE_TYPE_REGION1, granting the maximum 16 EB limit. Could this
allow an extremely restricted address limit to silently grant the maximum
possible address space?

> -		ret = -EBUSY;
> -		if (!kvm->created_vcpus)
> +		if (kvm->created_vcpus)
> +			return -EBUSY;
> +
> +		ret = 0;
> +		scoped_guard(mutex, &kvm->slots_lock) {
> +			slots = kvm_memslots(kvm);
> +			if (slots && !kvm_memslots_empty(slots)) {
> +				kvm_for_each_memslot(ms, bkt, slots) {
> +					if (gpa_to_gfn(new_limit) < ms->base_gfn + ms->npages) {
> +						ret = -EBUSY;
> +						break;
> +					}
> +				}
> +			}
> +		}
> +		if (!ret)
>  			ret = gmap_set_limit(kvm->arch.gmap, gpa_to_gfn(new_limit));

[Severity: High]
Is there a Time-of-Check to Time-of-Use (TOCTOU) race here by dropping
kvm->slots_lock before updating the ASCE limit?

By dropping slots_lock before calling gmap_set_limit(), the 64-bit
gmap->asce value is modified non-atomically under mmu_lock.

Concurrently, KVM_SET_USER_MEMORY_REGION can take slots_lock and call
kvm_arch_prepare_memory_region(), which reads gmap->asce locklessly:

kvm_arch_prepare_memory_region() {
    ...
    if (!asce_contains_gfn(kvm->arch.gmap->asce, ...))
        return -EINVAL;
}

Could this allow kvm_arch_prepare_memory_region() to read the old, larger
ASCE limit and validate a memslot that violates the newly requested limit?
Additionally, is there a risk of reading a torn gmap->asce struct value
during the non-atomic modification?

[ ... ]

> @@ -5672,6 +5692,8 @@ int kvm_arch_prepare_memory_region(struct kvm *kvm,
>  			return -EINVAL;
>  		if ((new->base_gfn + new->npages) * PAGE_SIZE > kvm->arch.mem_limit)
>  			return -EINVAL;
> +		if (!asce_contains_gfn(kvm->arch.gmap->asce, new->base_gfn + new->npages - 1))
> +			return -EINVAL;

[Severity: High]
Does this allow userspace to bypass the exact requested memory limit?

Earlier in kvm_s390_set_mem_control(), the exact new_limit (e.g., 1 GB)
is verified against existing memslots, but it is discarded and never stored
in kvm->arch.mem_limit.

gmap_set_limit() then converts the exact limit to a coarse ASCE boundary,
rounding it up to the next hardware limit (e.g., 2 GB).

Because this new check in kvm_arch_prepare_memory_region() only evaluates
against the coarse ASCE boundary via asce_contains_gfn(), could userspace
request a strict limit but silently allocate memslots up to the next
hardware boundary?

-- 
Sashiko AI review · https://sashiko.dev/#/patchset/20260601152951.196859-1-imbrenda@linux.ibm.com?part=8