Re: [PATCH v7 04/14] KVM: s390: Fix overclearing ESCA in case of error

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

Pre-existing issues:
- [Critical] Out-of-bounds ESCA entry accesses for ucontrol VMs due to bypassed vCPU ID limit check.
- [High] Memory leak of `vcpu->arch.mc` when `kvm_arch_vcpu_create` fails.
- [High] Memory leak in `kvm_s390_pv_create_cpu` error path allows malicious userspace to exhaust host memory (OOM).
--

--- Patch [4]: [PATCH v7 04/14] KVM: s390: Fix overclearing ESCA in case of error ---
commit 3c41df2f29ed108f81950295bc782ec27a142a6b
Author: Claudio Imbrenda <[email protected]>

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.

[Severity: High]
This isn't a bug introduced by this patch, but does this duplicated vCPU
error path leak memory?

If a duplicated vCPU ID causes kvm_arch_vcpu_create() to fail, the error
path jumps to out_free_sie_block:

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

This frees vcpu->arch.sie_block, but doesn't it leave vcpu->arch.mc
allocated? Can userspace intentionally exhaust host memory by repeatedly
creating duplicate vCPUs?


[Severity: High]
This is a pre-existing issue, but does a duplicate vCPU ID also trigger a
leak in the PV CPU creation error path?

If uv_call() fails due to the duplicate ID, kvm_s390_pv_create_cpu() calls
kvm_s390_pv_destroy_cpu():

arch/s390/kvm/pv.c:kvm_s390_pv_create_cpu() {
    ...
    cc = uv_call(0, (u64)&uvcb);
    if (cc) {
        u16 dummy;
        kvm_s390_pv_destroy_cpu(vcpu, &dummy, &dummy);
        return -EIO;
    }
}

Because uv_call() failed, the secure CPU handle is unpopulated (0). The
destroy function checks this handle and returns early:

arch/s390/kvm/pv.c:kvm_s390_pv_destroy_cpu() {
    if (!kvm_s390_pv_cpu_get_handle(vcpu))
        return 0;
    ...
}

Does this skip the free_pages() calls, abandoning the allocated
vcpu->arch.pv.stor_base and sida_addr?

> When destroying a vCPU, its ESCA entry will be cleared. In the above
> scenario, the spurious duplicate vCPU is destroyed, but the ESCA entry
> corresponding to the original vCPU is cleared.
> 
> Fix by skipping clearing the ESCA entry if the vCPU creation was not
> successful, i.e. if the pointer to the ESCA in the state description is
> not set.

> diff --git a/arch/s390/kvm/interrupt.c b/arch/s390/kvm/interrupt.c
> index 9e3e6b0d72ad6..2acdff130fa6c 100644
> --- a/arch/s390/kvm/interrupt.c
> +++ b/arch/s390/kvm/interrupt.c
> @@ -86,7 +86,7 @@ static void sca_clear_ext_call(struct kvm_vcpu *vcpu)
>  	struct esca_block *sca = vcpu->kvm->arch.sca;
>  	union esca_sigp_ctrl *sigp_ctrl = &sca->cpu[vcpu->vcpu_id].sigp_ctrl;
>  
> -	if (!kvm_s390_use_sca_entries())
> +	if (!kvm_s390_use_sca_entries() || !vcpu->arch.initialized)
>  		return;
>  	kvm_s390_clear_cpuflags(vcpu, CPUSTAT_ECALL_PEND);

[Severity: Critical]
This isn't a bug introduced by this patch, but does this function perform
an out-of-bounds write for ucontrol VMs?

For ucontrol VMs, kvm_arch_vcpu_precreate() skips the sca_can_add_vcpu()
bounds check, allowing vcpu_id to reach up to 254 (KVM_MAX_VCPU_IDS - 1). 
However, the ESCA block (sca->cpu) only contains 248 slots.

When a vcpu_id is >= 248, does the subsequent write to sigp_ctrl overwrite
adjacent memory?

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

Similar out-of-bounds access happens in sca_inject_ext_call() during
cmpxchg(). Can an unprivileged user exploit this by creating a ucontrol VM
with vcpu_id >= 248 and triggering vCPU teardown?

[ ... ]

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