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

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

Pre-existing issues:
- [High] Missing ucontrol checks in ESCA handling lead to out-of-bounds memory accesses for KVM_VM_UCONTROL VMs.
- [High] Memory leak of `vcpu->arch.mc` on error paths in `kvm_arch_vcpu_create()`.
--

--- Patch [3]: [PATCH v6 03/14] KVM: s390: Fix overclearing ESCA in case of error ---
commit 0972b46b8d10201faec4d5ded093d7c4cc013c9b
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. 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.

> 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)

[Severity: High]
This is a pre-existing issue, but missing ucontrol checks in ESCA handling
can lead to out-of-bounds memory accesses for KVM_VM_UCONTROL VMs.

If a user creates a KVM_VM_S390_UCONTROL VM (requires CAP_SYS_ADMIN), skips
sca_can_add_vcpu() limit checks during vCPU creation, and successfully
initializes vCPU IDs up to 254 (above the ESCA array limit of 248), does this
allow out-of-bounds reads and writes?

For example, when injecting KVM_S390_INT_EXTERNAL_CALL or resetting the vCPU,
sca_clear_ext_call() unconditionally writes 0:

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

Similarly, sca_inject_ext_call() performs an out-of-bounds cmpxchg write:

arch/s390/kvm/interrupt.c:sca_inject_ext_call() {
    ...
    rc = cmpxchg(&sigp_ctrl->value, old_val.value, new_val.value);
}

Could this corrupt the kernel memory adjacent to the esca_block pages for
vCPU IDs >= 248?

>  		return;
>  	kvm_s390_clear_cpuflags(vcpu, CPUSTAT_ECALL_PEND);

[ ... ]

> diff --git a/arch/s390/kvm/kvm-s390.c b/arch/s390/kvm/kvm-s390.c
> index 9ec5833a09732..58cec3703c2d5 100644
> --- a/arch/s390/kvm/kvm-s390.c
> +++ b/arch/s390/kvm/kvm-s390.c

[ ... ]

[Severity: High]
This is a pre-existing issue, but does kvm_arch_vcpu_create() in
arch/s390/kvm/kvm-s390.c leak the MMU cache allocation on error paths?

If a subsequent allocation fails after kvm_s390_new_mmu_cache() assigns the
cache to vcpu->arch.mc, the function jumps to out_free_sie_block and returns
an error without freeing the MMU cache:

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

Since kvm_vm_ioctl_create_vcpu() decrements kvm->created_vcpus and frees the
vCPU struct but does not call kvm_arch_vcpu_destroy() on this error path,
would vcpu->arch.mc be permanently leaked?

Can an unprivileged user repeatedly attempt to create a vCPU and force memory
allocation failures to cause an unbounded memory leak?

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