Re: [PATCH v1 2/8] KVM: s390: Fix unlikely NULL gmap dereference

[sashiko-bot@kernel.org]

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

Pre-existing issues:
- [Critical] The patch incompletely fixes the race condition between vCPU creation and `KVM_RUN`. While it fixes the `gmap` NULL pointer dereference, it leaves the initialization of other critical `sie_block` fields (`sca_add_vcpu`, `epoch`, `epdx`, `ictl`) in `kvm_arch_vcpu_postcreate()`, which can run concurrently with `KVM_RUN` and corrupt host memory.
- [High] Calling `gmap_remove_child()` without holding `children_lock` in `kvm_arch_vcpu_create()` error path.
- [High] Concurrent creation of vCPUs with the same `vcpu_id` clears the valid vCPU's SCA entry.
--

commit 8b61d8766d5d3ccd96fcdcaf7ad4379e712b4282
Author: Claudio Imbrenda <imbrenda@linux.ibm.com>

KVM: s390: Fix unlikely NULL gmap dereference

When creating a new vCPU, kvm_vm_ioctl_create_vcpu() will call
kvm_arch_vcpu_postcreate() after the file descriptor for the new vCPU
has been created. The new file descriptor has not been returned yet,
but a malicious userspace program could try to guess it.

> diff --git a/arch/s390/kvm/kvm-s390.c b/arch/s390/kvm/kvm-s390.c
> index 23c817595e28d..4a6d903e35231 100644
> --- a/arch/s390/kvm/kvm-s390.c
> +++ b/arch/s390/kvm/kvm-s390.c
> @@ -3604,10 +3604,8 @@ void kvm_arch_vcpu_postcreate(struct kvm_vcpu *vcpu)
>  	vcpu->arch.sie_block->epdx = vcpu->kvm->arch.epdx;
>  	preempt_enable();
>  	mutex_unlock(&vcpu->kvm->lock);
> -	if (!kvm_is_ucontrol(vcpu->kvm)) {
> -		vcpu->arch.gmap = vcpu->kvm->arch.gmap;
> +	if (!kvm_is_ucontrol(vcpu->kvm))
>  		sca_add_vcpu(vcpu);

[Severity: High]
This isn't a bug introduced by this patch, but does leaving sca_add_vcpu() and
other sie_block initialization in kvm_arch_vcpu_postcreate() leave a race
condition open?

The commit message notes that malicious userspace can start the newly created
vCPU before kvm_arch_vcpu_postcreate() finishes. If kvm_vm_ioctl_create_vcpu()
drops vcpu->mutex and kvm->lock before calling kvm_arch_vcpu_postcreate(),
can KVM_RUN execute concurrently?

If KVM_RUN enters the guest before sca_add_vcpu() completes:

arch/s390/kvm/kvm-s390.c:sca_add_vcpu() {
	...
	vcpu->arch.sie_block->scaoh = sca_phys >> 32;
	vcpu->arch.sie_block->scaol = sca_phys & ESCA_SCAOL_MASK;
	vcpu->arch.sie_block->ecb2 |= ECB2_ESCA;
	...
}

Will the hardware use physical address 0 as the System Control Area base and
overwrite host absolute physical address 0 when updating inter-processor
states?

>  	if (test_kvm_facility(vcpu->kvm, 74) || vcpu->kvm->arch.user_instr0 ||
>  	    vcpu->kvm->arch.user_operexec)
>  		vcpu->arch.sie_block->ictl |= ICTL_OPEREXC;

[ ... ]

> @@ -3850,6 +3848,8 @@ int kvm_arch_vcpu_create(struct kvm_vcpu *vcpu)
>  		vcpu->arch.gmap = gmap_new_child(vcpu->kvm->arch.gmap, -1UL);
>  		if (!vcpu->arch.gmap)
>  			goto out_free_sie_block;
> +	} else {
> +		vcpu->arch.gmap = vcpu->kvm->arch.gmap;
>  	}
>  
>  	VM_EVENT(vcpu->kvm, 3, "create cpu %d at 0x%p, sie block at 0x%p",

[Severity: High]
This is a pre-existing issue, but does the error handling path for ucontrol
vCPUs in kvm_arch_vcpu_create() safely remove the gmap child?

If kvm_s390_vcpu_setup() fails, the out_ucontrol_uninit path calls
gmap_remove_child(vcpu->arch.gmap). Looking at gmap_remove_child() in
arch/s390/kvm/gmap.c, it modifies child->list and has a lockdep assertion
for child->parent->children_lock:

arch/s390/kvm/gmap.c:gmap_remove_child() {
	lockdep_assert_held(&child->parent->children_lock);

	list_del(&child->list);
	...
}

Since kvm_arch_vcpu_create() does not acquire children_lock before jumping to
out_ucontrol_uninit, could this cause list corruption if concurrent creation
failures race?

[Severity: High]
This is also a pre-existing issue, but is there a risk of clearing a valid
SCA entry if userspace concurrently creates two vCPUs with the same vcpu_id?

Duplicate ID detection occurs in kvm_vm_ioctl_create_vcpu() after
kvm_arch_vcpu_create() succeeds. If the ID check fails, it eventually calls
kvm_arch_vcpu_destroy(), which unconditionally calls sca_del_vcpu():

arch/s390/kvm/kvm-s390.c:sca_del_vcpu() {
	...
	sca->cpu[vcpu->vcpu_id].sda = 0;
	...
}

If a valid vCPU already exists with this vcpu_id, does the failed creation
thread wipe out the valid vCPU's SCA interrupt delivery structures?

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