Re: [PATCH v2 6/7] KVM: s390: Fix overclearing ESCA in case of error

[sashiko-bot@kernel.org]

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

New issues:
- [High] The condition `!vcpu->arch.sie_block->scaol` incorrectly assumes `scaol` is always non-zero for a valid SCA physical address, which can lead to silently skipping ESCA cleanup for valid vCPUs.

Pre-existing issues:
- [Critical] In `sca_add_vcpu()`, the hardware-visible ESCA `mcn` validity bit is set before the `sda` (State Description Address) pointer is written, exposing an uninitialized (zero) `sda` to concurrent hardware SIGP operations.
- [High] The patch fixes the clearing of the ESCA entry in `sca_del_vcpu()` during duplicate vCPU destruction, but misses an identical issue in `sca_clear_ext_call()` which also corrupts the original vCPU's ESCA state.
- [High] Memory leak of `vcpu->arch.mc` on the error path of `kvm_arch_vcpu_create()`.
--

--- Patch [6]: [PATCH v2 6/7] KVM: s390: Fix overclearing ESCA in case of error ---
commit 2c6e0529648df791186dd5602fe55245586d90d7
Author: Claudio Imbrenda <imbrenda@linux.ibm.com>

KVM: s390: Fix overclearing ESCA in case of error
    
If an attempt is made to create a vCPU with an already existing ID,
the duplicated vCPU will be destroyed. When destroying a vCPU, its
ESCA entry will be cleared. This patch fixes a regression where the
original vCPU ESCA entry was erroneously cleared by checking if the
pointer to the ESCA is set.

> diff --git a/arch/s390/kvm/kvm-s390.c b/arch/s390/kvm/kvm-s390.c
> index fc0a884d7f549..6339a327a7be2 100644
> --- a/arch/s390/kvm/kvm-s390.c
> +++ b/arch/s390/kvm/kvm-s390.c
> @@ -3458,7 +3458,7 @@ static void sca_del_vcpu(struct kvm_vcpu *vcpu)
>  {
>  	struct esca_block *sca = vcpu->kvm->arch.sca;
>  
> -	if (!kvm_s390_use_sca_entries())
> +	if (!kvm_s390_use_sca_entries() || !vcpu->arch.sie_block->scaol)

[Severity: High]
Is it possible for scaol to legitimately be zero here?

Since scaol stores the lower 32 bits of the ESCA block physical address, if
the block allocated via alloc_pages_exact() happens to be 4GB aligned
(e.g., 0x100000000), scaol would evaluate to zero.

Would this cause the cleanup to be skipped for a fully initialized vCPU,
leaving the mcn bit set and sda pointing to a freed sie_block? Can this
cause a use-after-free if hardware receives a SIGP for the freed vCPU ID?

>  		return;
>  
>  	clear_bit_inv(vcpu->vcpu_id, (unsigned long *)sca->mcn);

[Severity: High]
This is a pre-existing issue, but it looks like a similar regression exists
in sca_clear_ext_call().

When a duplicate vCPU is destroyed, kvm_arch_vcpu_destroy() calls
kvm_s390_clear_local_irqs(), which invokes sca_clear_ext_call(). Without a
similar initialization check, this unconditionally zeroes the sigp_ctrl:

arch/s390/kvm/interrupt.c:sca_clear_ext_call() {
    ...
    WRITE_ONCE(sigp_ctrl->value, 0);
    ...
}

Will this silently drop pending external calls or emergency signals for the
original, legitimate vCPU?


[Severity: High]
This is a pre-existing issue, but I noticed a potential memory leak on the
error path of kvm_arch_vcpu_create().

If an initialization step fails after vcpu->arch.mc is allocated via
kvm_s390_new_mmu_cache(), the error path frees vcpu->arch.sie_block but
appears to omit freeing vcpu->arch.mc:

arch/s390/kvm/kvm-s390.c:kvm_arch_vcpu_create() {
    ...
out_free_sie_block:
    free_page((unsigned long)(vcpu->arch.sie_block));
    return rc;
}

Does this code leak the memory allocated for the mmu_cache?


[Severity: Critical]
This is a pre-existing issue, but the initialization order in sca_add_vcpu()
seems risky.

The mcn validity bitmask is set before the sda pointer is written:

arch/s390/kvm/kvm-s390.c:sca_add_vcpu() {
    ...
    set_bit_inv(vcpu->vcpu_id, (unsigned long *)sca->mcn);
    sca->cpu[vcpu->vcpu_id].sda = virt_to_phys(vcpu->arch.sie_block);
    ...
}

Can a concurrent hardware SIGP instruction from another vCPU observe the
valid mcn bit and read an uninitialized sda pointer before the host software
writes the SIE block address?

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