Re: [PATCH v4 06/13] KVM: s390: Fix overclearing ESCA in case of error

[sashiko-bot@kernel.org]

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

Pre-existing issues:
- [High] Memory leak of MMU cache on kvm_arch_vcpu_create() error path.
- [High] Out-of-bounds array access and kernel memory corruption in ESCA interrupt handlers for UCONTROL VMs due to unconstrained vcpu_id.
--

--- Patch [6]: [PATCH v4 06/13] KVM: s390: Fix overclearing ESCA in case of error ---
commit a40c7240ce30546ef77f030cf591a1bc94d9f75b
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. This patch avoids clearing the ESCA
entry if the vCPU creation was not successful.

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

[Severity: High]
This is a pre-existing issue, but does this pointer calculation allow an
out-of-bounds access for UCONTROL VMs?

UCONTROL VMs bypass the sca_can_add_vcpu() bounds check during
kvm_arch_vcpu_precreate(), which allows vcpu_id to reach up to 254 instead
of the array limit of 247.

When a successfully initialized UCONTROL vCPU is destroyed,
kvm_arch_vcpu_destroy() unconditionally calls kvm_s390_clear_local_irqs(),
which invokes sca_clear_ext_call().

Because the sigp_ctrl pointer is computed without verifying if the VM is a
UCONTROL VM, a vcpu_id of 254 resolves to an offset 132 bytes beyond the
allocated esca_block.

Could the subsequent WRITE_ONCE(sigp_ctrl->value, 0) further down in the
function then overwrite adjacent kernel memory?

>  	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 fa6a0bacd9675..24b48dc5655b1 100644
> --- a/arch/s390/kvm/kvm-s390.c
> +++ b/arch/s390/kvm/kvm-s390.c
> @@ -3459,7 +3459,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.initialized)
>  		return;

[Severity: High]
This isn't a bug introduced by this patch, but does the error path for vCPU 
creation leak the MMU cache?

In kvm_arch_vcpu_create(), vcpu->arch.mc is dynamically allocated:

arch/s390/kvm/kvm-s390.c:kvm_arch_vcpu_create() {
    ...
    vcpu->arch.mc = kvm_s390_new_mmu_cache();
    ...
}

If a subsequent initialization step fails (such as gmap_new_child() failing
for UCONTROL VMs, or kvm_s390_vcpu_setup() returning an error), the function
jumps to the out_free_sie_block label.

While this error path frees vcpu->arch.sie_block, it completely neglects to
call kvm_s390_free_mmu_cache(vcpu->arch.mc).

Since the caller (kvm_vm_ioctl_create_vcpu()) then directly frees the vcpu
struct without calling kvm_arch_vcpu_destroy(), does this permanently leak
the kvm_s390_mmu_cache and its associated page tables?

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

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