Re: [PATCH v4 08/13] KVM: s390: Free the mmu cache when kvm_arch_vcpu_create() fails

[sashiko-bot@kernel.org]

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

Pre-existing issues:
- [Critical] Unconditional call to `kvm_s390_vcpu_unsetup_cmma()` when PV VCPU creation fails causes freeing of physical page 0 if CMMA is disabled, leading to a host panic.
--

--- Patch [8]: [PATCH v4 08/13] KVM: s390: Free the mmu cache when kvm_arch_vcpu_create() fails ---
commit 30d38fd7067b631b7997fead40df5a9ae47fcfb8
Author: Claudio Imbrenda <imbrenda@linux.ibm.com>

KVM: s390: Free the mmu cache when kvm_arch_vcpu_create() fails

This commit fixes a memory leak where the mmu cache is not freed if
vcpu creation fails. It also refactors kvm_arch_vcpu_create() to use
scope-based cleanup instead of goto-based error handling.

> 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

[ ... ]

> @@ -3849,10 +3849,9 @@ int kvm_arch_vcpu_create(struct kvm_vcpu *vcpu)
>  	VM_EVENT(vcpu->kvm, 3, "create cpu %d at 0x%p, sie block at 0x%p",
>  		 vcpu->vcpu_id, vcpu, vcpu->arch.sie_block);
>  	trace_kvm_s390_create_vcpu(vcpu->vcpu_id, vcpu, vcpu->arch.sie_block);
>  
>  	rc = kvm_s390_vcpu_setup(vcpu);

[Severity: Critical]
This is a pre-existing issue, but could this lead to a host panic if a
Protected VM fails to create a VCPU and CMMA is disabled?

If kvm_s390_pv_create_cpu() fails (e.g., hitting a cgroup memory limit)
inside kvm_s390_vcpu_setup(), it unconditionally unsets CMMA:

kvm_s390_vcpu_setup() {
    ...
	if (kvm_s390_pv_is_protected(vcpu->kvm)) {
		rc = kvm_s390_pv_create_cpu(vcpu, &uvrc, &uvrrc);
		if (rc)
			kvm_s390_vcpu_unsetup_cmma(vcpu);
	}
    ...
}

If use_cmma is false, vcpu->arch.sie_block->cbrlo is 0. 
kvm_s390_vcpu_unsetup_cmma() then executes:

kvm_s390_vcpu_unsetup_cmma() {
    ...
	free_page((unsigned long)phys_to_virt(vcpu->arch.sie_block->cbrlo));
    ...
}

With KASLR enabled, phys_to_virt(0) evaluates to __identity_base 
(a non-zero address), which bypasses the safety check in free_pages(). 
Does this erroneously free physical page 0 (the lowcore page), leading 
to memory corruption or a fatal VM_BUG_ON_PAGE panic on the host?

> -	if (rc)
> -		goto out_ucontrol_uninit;
> +	if (rc) {
> +		if (kvm_is_ucontrol(vcpu->kvm)) {
> +			scoped_guard(spinlock, &vcpu->kvm->arch.gmap->children_lock)
> +				gmap_remove_child(vcpu->arch.gmap);
> +			vcpu->arch.gmap = gmap_put(vcpu->arch.gmap);
> +		}
> +		return rc;
> +	}
>  
> +	vcpu->arch.mc = no_free_ptr(mc);
> +	sie_page = NULL;
>  	kvm_s390_update_topology_change_report(vcpu->kvm, 1);
>  	return 0;

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