Re: [PATCH v3 05/10] KVM: s390: ucontrol: Add missing locking around gmap_remove_child()

[sashiko-bot@kernel.org]

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

Pre-existing issues:
- [Critical] Unconditional free of physical page 0 on error path in kvm_s390_vcpu_setup() when CMMA is disabled.
--

--- Patch [5]: [PATCH v3 05/10] KVM: s390: ucontrol: Add missing locking around gmap_remove_child() ---
commit 23ca669df08a6ebee3b62a02dbada5159020162e
Author: Claudio Imbrenda <imbrenda@linux.ibm.com>

KVM: s390: ucontrol: Add missing locking around gmap_remove_child()

gmap_remove_child() needs to be called while holding the children_lock
of the parent gmap. This was not the case in the error handling path of
kvm_arch_vcpu_create() for UCONTROL guests.

> diff --git a/arch/s390/kvm/kvm-s390.c b/arch/s390/kvm/kvm-s390.c
> index 0a44351722702..e2a15799b9ac3 100644
> --- a/arch/s390/kvm/kvm-s390.c
> +++ b/arch/s390/kvm/kvm-s390.c
> @@ -3867,7 +3867,9 @@ int kvm_arch_vcpu_create(struct kvm_vcpu *vcpu)
>  
>  out_ucontrol_uninit:

[Severity: Critical]
This is a pre-existing issue, but is it possible for the error handling path
in kvm_s390_vcpu_setup() to accidentally free physical page 0?

In kvm_s390_vcpu_setup(), if use_cmma is 0, kvm_s390_vcpu_setup_cmma() is
skipped and vcpu->arch.sie_block->cbrlo is left at its initialized value
of 0.

If a subsequent step like kvm_s390_pv_create_cpu() fails, the error path
unconditionally triggers the CMMA cleanup:

arch/s390/kvm/kvm-s390.c:kvm_s390_vcpu_setup() {
    ...
    if (rc)
        goto out_free_cpu;
    ...
out_free_cpu:
    kvm_s390_vcpu_unsetup_cmma(vcpu);
    ...
}

Inside kvm_s390_vcpu_unsetup_cmma(), the page is freed without checking if
CMMA was actually initialized:

arch/s390/kvm/kvm-s390.c:kvm_s390_vcpu_unsetup_cmma() {
    ...
    free_page((unsigned long)phys_to_virt(vcpu->arch.sie_block->cbrlo));
    ...
}

On s390, phys_to_virt(0) evaluates to __identity_base. When KASLR is enabled,
this is a non-zero virtual address that maps exactly to physical page 0.

Because the resulting virtual address is non-zero, it bypasses the addr != 0
safeguard in free_pages(), which could cause physical page 0 (containing
s390 lowcore data) to be freed into the buddy allocator.

Would it make sense to verify use_cmma is enabled or that cbrlo is non-zero
before freeing this page?

>  	if (kvm_is_ucontrol(vcpu->kvm)) {
> +		spin_lock(&vcpu->kvm->arch.gmap->children_lock);
>  		gmap_remove_child(vcpu->arch.gmap);
> +		spin_unlock(&vcpu->kvm->arch.gmap->children_lock);
>  		vcpu->arch.gmap = gmap_put(vcpu->arch.gmap);
>  	}
>  out_free_sie_block:

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