Re: [PATCH v5 13/13] KVM: s390: Fix kvm_arch_commit_memory_region() when low on memory

Thank you for your contribution! Sashiko AI review found 2 potential issue(s) to consider:
- [Critical] Dropping `kvm->mmu_lock` and retrying allows guest page faults to populate page tables that are subsequently freed by `dat_create_slot`, destroying guest ESSA state and leaking `cmma_dirty_pages`.
- [High] The patch introduces a host-userspace-reachable KVM_BUG_ON() which triggers a WARN_ON_ONCE, allowing unprivileged users to spam the kernel log or crash the host if panic_on_warn is set.
--

--- Patch [13]: [PATCH v5 13/13] KVM: s390: Fix kvm_arch_commit_memory_region() when low on memory ---
commit 4801c2be100c4b8f582c02a4637cf4703c366817
Author: Claudio Imbrenda <[email protected]>

KVM: s390: Fix kvm_arch_commit_memory_region() when low on memory
    
This commit updates the out-of-memory handling in
kvm_arch_commit_memory_region(). It refills the kvm_s390_mmu_cache and
retries the operation, and warns and kills the VM if the refill fails.

> --- a/arch/s390/kvm/kvm-s390.c
> +++ b/arch/s390/kvm/kvm-s390.c
[ ... ]
> @@ -5822,7 +5822,7 @@ void kvm_arch_commit_memory_region(struct kvm *kvm,
[ ... ]
>  	mc = kvm_s390_new_mmu_cache();
> -	if (!mc) {
> -		rc = -ENOMEM;
> +	if (!mc)
>  		goto out;
> -	}
> -
> +retry:
>  	scoped_guard(write_lock, &kvm->mmu_lock) {
[ ... ]
>  		default:
> +			rc = 0;
>  			WARN(1, "Unknown KVM MR CHANGE: %d\n", change);
>  		}
>  	}
> +	if (rc == -ENOMEM) {
> +		rc = kvm_s390_mmu_cache_topup(mc);
> +		if (!rc)
> +			goto retry;

[Severity: Critical]
Does dropping kvm->mmu_lock between the scoped_guard exit and the goto
retry allow concurrent guest page faults?

If a guest vCPU faults on an address in the new memslot during this
window, it could populate a page table and execute ESSA instructions.
When kvm_arch_commit_memory_region() restarts dat_create_slot() after
re-acquiring the lock, could it blindly replace the now-populated CRSTE
and free the new page table without cleaning up the atomic counters?

Looking at dat_create_slot(), it appears that it replaces the CRSTE and
frees the lower-level page table:

arch/s390/kvm/dat.c:_dat_slot_crste() {
    ...
		/* A lower level table was present, needs to be freed. */
		if (!crste.h.fc && !crste.h.i) {
			if (is_pmd(crste))
				dat_free_pt(dereference_pmd(crste.pmd));
    ...
}

Could this regression result in the loss of guest state and leak
cmma_dirty_pages?

> +	}
>  out:
> -	if (rc)
> +	if (KVM_BUG_ON(rc, kvm))

[Severity: High]
Can this KVM_BUG_ON be triggered by an unprivileged host userspace
process?

If a process in a restrictive memory cgroup invokes the memslot ioctl,
kvm_s390_mmu_cache_topup() could fail with -ENOMEM, passing the error to
this path. Since KVM_BUG_ON internally executes WARN_ON_ONCE, could this
allow a malicious host-userspace process to spam the kernel log or
trigger a crash if panic_on_warn is set?

>  		pr_warn("failed to commit memory region\n");
>  	return;
>  }

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