Re: [PATCH v5 03/13] 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:
- [Critical] Creating and destroying a KVM_VM_S390_UCONTROL VM with vCPU IDs >= KVM_S390_ESCA_CPU_SLOTS (248) causes out-of-bounds memory accesses on `esca_block->cpu`.
- [High] gmap_remove_child() is called without holding the parent gmap's children_lock in the error path of kvm_arch_vcpu_create().
--

--- Patch [3]: [PATCH v5 03/13] KVM: s390: Fix overclearing ESCA in case of error ---
commit de5b0e7f34127f8db5eb8b0fa184043139c25a8c
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 is destroyed. This patch ensures the ESCA entry
is only cleared if the vCPU was successfully created to avoid
clearing the original vCPU's ESCA entry by mistake.

> 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: Critical]
This is a pre-existing issue, but can this out-of-bounds access still occur
for UCONTROL VMs?

In kvm_arch_vcpu_precreate(), the sca_can_add_vcpu() bounds check is skipped
for UCONTROL VMs, which allows vcpu_id to be up to KVM_MAX_VCPUS - 1 (254):

arch/s390/kvm/kvm-s390.c:kvm_arch_vcpu_precreate() {
	...
	if (!kvm_is_ucontrol(kvm) && !sca_can_add_vcpu(kvm, id))
		return -EINVAL;
	...
}

Because the esca_block->cpu array only has KVM_S390_ESCA_CPU_SLOTS (248)
elements, if a UCONTROL vCPU with an ID between 248 and 254 successfully
initializes, it would bypass the new !vcpu->arch.initialized check here.

When this vCPU is destroyed in sca_clear_ext_call() or handles an external call
interrupt in sca_inject_ext_call(), computing &sca->cpu[vcpu->vcpu_id] results
in a pointer outside the allocated esca_block.

Could this allow writing out of bounds when WRITE_ONCE(sigp_ctrl->value, 0) is
called?

>  	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 096b426c1281b..3e40a52bfd152 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;
>  
>  	clear_bit_inv(vcpu->vcpu_id, (unsigned long *)sca->mcn);

[Severity: High]
This isn't a bug introduced by this patch, but is there a data race in the
error handling path of kvm_arch_vcpu_create() for UCONTROL VMs?

If kvm_s390_vcpu_setup() fails, the code jumps to out_ucontrol_uninit:

arch/s390/kvm/kvm-s390.c:kvm_arch_vcpu_create() {
	...
out_ucontrol_uninit:
	if (kvm_is_ucontrol(vcpu->kvm)) {
		gmap_remove_child(vcpu->arch.gmap);
		vcpu->arch.gmap = gmap_put(vcpu->arch.gmap);
	}
	...
}

Because kvm->lock is released prior to calling kvm_arch_vcpu_create(), multiple
concurrent vCPU creations could fail and execute this error path simultaneously.

Since gmap_remove_child() mutates the parent gmap's children list without
holding the children_lock, could this lead to list corruption?

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