Re: [PATCH v5 05/13] KVM: s390: Fix leaking of PGM_ADDRESSING to userspace

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

Pre-existing issues:
- [High] O(N^2) execution and potential soft lockup in `kvm_s390_set_skeys` and `kvm_s390_set_cmma_bits` due to restarting the iteration from `0` on every MMU cache top-up.
--

--- Patch [5]: [PATCH v5 05/13] KVM: s390: Fix leaking of PGM_ADDRESSING to userspace ---
commit 89e2ddf835cf981fcd7459eaa6f4853002045254
Author: Claudio Imbrenda <[email protected]>

KVM: s390: Fix leaking of PGM_ADDRESSING to userspace

This commit fixes KVM operations like kvm_s390_set_skeys and
kvm_s390_set_cmma_bits by converting positive error codes (such as
PGM_ADDRESSING) to -EFAULT. This ensures negative error values are properly
returned to userspace.

> diff --git a/arch/s390/kvm/kvm-s390.c b/arch/s390/kvm/kvm-s390.c
> --- a/arch/s390/kvm/kvm-s390.c
> +++ b/arch/s390/kvm/kvm-s390.c

[ ... ]

> @@ -2274,7 +2274,7 @@ static int kvm_s390_set_skeys(struct kvm *kvm, struct kvm_s390_skeys *args)
>  	kvm_s390_free_mmu_cache(mc);
>  out:
>  	kvfree(keys);
> -	return r;
> +	return r <= 0 ? r : -EFAULT;
>  }

[Severity: High]
This is a pre-existing issue, but does the iteration in kvm_s390_set_skeys()
risk a soft lockup?

If dat_set_storage_key() returns -ENOMEM because the MMU cache is depleted,
the outer do-while loop tops up the cache and retries. However, the inner
for-loop unconditionally restarts from i = 0:

        scoped_guard(read_lock, &kvm->mmu_lock) {
                for (i = 0 ; i < args->count; i++) {
                        r = dat_set_storage_key(mc, kvm->arch.gmap->asce,
                                                args->start_gfn + i, keys[i], 0);

If userspace passes a large count (up to KVM_S390_SKEYS_MAX), restarting from
0 on every page table allocation could result in O(N^2) execution time. 

Since this is done while holding the mmu_lock with preemption disabled, could
this trigger RCU stalls or kernel panics?

[ ... ]

> @@ -2384,7 +2384,7 @@ static int kvm_s390_set_cmma_bits(struct kvm *kvm,
>  
>  	set_bit(GMAP_FLAG_USES_CMM, &kvm->arch.gmap->flags);
>  
> -	return r;
> +	return r <= 0 ? r : -EFAULT;
>  }

[Severity: High]
This is also a pre-existing issue, but does kvm_s390_set_cmma_bits() have
the same quadratic restart problem?

Similar to the skeys function above, if dat_set_cmma_bits() returns -ENOMEM,
the loop tops up the cache and restarts processing from args->start_gfn:

        do {
                r = kvm_s390_mmu_cache_topup(mc);
                if (r)
                        return r;
                scoped_guard(read_lock, &kvm->mmu_lock) {
                        r = dat_set_cmma_bits(mc, kvm->arch.gmap->asce, args->start_gfn,
                                              args->count, args->mask, bits);
                }
        } while (r == -ENOMEM);

Should these loops advance the starting address/index so they resume where
they left off instead of starting over?

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