Re: [PATCH v6 04/14] KVM: s390: ucontrol: Fix sca_clear_ext_call()

Thank you for your contribution! Sashiko AI review found 1 potential issue(s) to consider:
- [Medium] The patch introduces a KVM_BUG_ON() in sca_inject_ext_call() and sca_ext_call_pending() to assert that UCONTROL VMs never reach these functions. However, userspace can easily reach them because the patch fails to fence VCPU-level ioctls and memory-mapped sie_block access.
--

--- Patch [4]: [PATCH v6 04/14] KVM: s390: ucontrol: Fix sca_clear_ext_call() ---
commit b27149eacddb40ae95c3a0b8dc6d91809e9ace8b
Author: Claudio Imbrenda <[email protected]>

KVM: s390: ucontrol: Fix sca_clear_ext_call()

When cleaning up a UCONTROL VM, sca_clear_ext_call() will touch memory
outside of the allocated ESCA block, and UCONTROL VMs don't even use
ESCA. This patch fixes the issue by not touching ESCA for UCONTROL VMs
and fencing the VM-level KVM_S390_INTERRUPT ioctl. It also adds checks
to prevent sca_ext_call_pending() and sca_inject_ext_call() from running
on UCONTROL VMs.

> diff --git a/arch/s390/kvm/interrupt.c b/arch/s390/kvm/interrupt.c
> index 2acdff130fa6c..6013f28e3461f 100644
> --- a/arch/s390/kvm/interrupt.c
> +++ b/arch/s390/kvm/interrupt.c
> @@ -45,13 +45,16 @@ static struct kvm_s390_gib *gib;
>  static int sca_ext_call_pending(struct kvm_vcpu *vcpu, int *src_id)
>  {
>  	struct esca_block *sca = vcpu->kvm->arch.sca;
> -	union esca_sigp_ctrl sigp_ctrl = sca->cpu[vcpu->vcpu_id].sigp_ctrl;
> +	union esca_sigp_ctrl sigp_ctrl;
>  
>  	if (!kvm_s390_test_cpuflags(vcpu, CPUSTAT_ECALL_PEND))
>  		return 0;
>  
> +	if (KVM_BUG_ON(kvm_is_ucontrol(vcpu->kvm), vcpu->kvm))
> +		return -EINVAL;

[Severity: Medium]
Is it possible for userspace to trigger this KVM_BUG_ON and cause a host
kernel warning splat?

For UCONTROL VMs, the sie_block is directly memory-mapped to userspace at
KVM_S390_SIE_PAGE_OFFSET. If userspace manually sets the CPUSTAT_ECALL_PEND
bit in sie_block->cpuflags and executes KVM_RUN, wouldn't it reach this
check during pending interrupt evaluation?

>  	BUG_ON(!kvm_s390_use_sca_entries());

[ ... ]

> @@ -60,13 +63,16 @@ static int sca_ext_call_pending(struct kvm_vcpu *vcpu, int *src_id)
>  
>  static int sca_inject_ext_call(struct kvm_vcpu *vcpu, int src_id)
>  {
> -	struct esca_block *sca = vcpu->kvm->arch.sca;
> -	union esca_sigp_ctrl *sigp_ctrl = &sca->cpu[vcpu->vcpu_id].sigp_ctrl;
>  	union esca_sigp_ctrl old_val, new_val = {.scn = src_id, .c = 1};
> +	struct esca_block *sca = vcpu->kvm->arch.sca;
> +	union esca_sigp_ctrl *sigp_ctrl;
>  	int expect, rc;
>  
>  	BUG_ON(!kvm_s390_use_sca_entries());
> +	if (KVM_BUG_ON(kvm_is_ucontrol(vcpu->kvm), vcpu->kvm))
> +		return -EINVAL;

[Severity: Medium]
Could this KVM_BUG_ON be reached via the VCPU-level ioctls?

While this patch fences the VM-level KVM_S390_INTERRUPT ioctl, it appears
that the VCPU-level KVM_S390_IRQ and KVM_S390_INTERRUPT ioctls in
kvm_arch_vcpu_unlocked_ioctl() remain unfenced for UCONTROL VMs.

If userspace calls one of these VCPU ioctls with KVM_S390_INT_EXTERNAL_CALL,
would it flow through kvm_s390_inject_vcpu() to sca_inject_ext_call() and hit
this assertion?

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