Re: [PATCH v7 05/14] KVM: s390: ucontrol: Fix sca_clear_ext_call()

Thank you for your contribution! Sashiko AI review found 2 potential issue(s) to consider:
- [High] Host userspace and malicious guests can trigger a KVM_BUG_ON (WARN_ON_ONCE) by injecting an external call into a UCONTROL VM.
- [High] Fencing the KVM_S390_INTERRUPT ioctl entirely for UCONTROL VMs breaks backwards compatibility.
--

--- Patch [5]: [PATCH v7 05/14] KVM: s390: ucontrol: Fix sca_clear_ext_call() ---
commit 22db5ca2b2a238d3fc521f6c7e38a951435c51a0
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 commit stops UCONTROL VMs from touching ESCA, fences the
KVM_S390_INTERRUPT ioctl, and adds checks to other ESCA paths.

> diff --git a/arch/s390/kvm/interrupt.c b/arch/s390/kvm/interrupt.c
> index 2acdff130fa6c..9db68e3ac178f 100644
> --- a/arch/s390/kvm/interrupt.c
> +++ b/arch/s390/kvm/interrupt.c

[ ... ]

> @@ -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: High]
Can a malicious guest or unprivileged host userspace process trigger this
assertion?

It appears UCONTROL VMs can still reach sca_inject_ext_call() through other
unfenced paths. For example, host userspace can call the KVM_S390_IRQ ioctl
with KVM_S390_INT_EXTERNAL_CALL without being fenced, as the UCONTROL check
was only added to KVM_S390_INTERRUPT. 

The ioctl flows through:
kvm_arch_vcpu_unlocked_ioctl()
  case KVM_S390_IRQ:
    kvm_s390_inject_vcpu()
      do_inject_vcpu()
        sca_inject_ext_call()

Similarly, a guest executing a SIGP EXTERNAL CALL instruction can intercept
to __sigp_external_call(), which also calls kvm_s390_inject_vcpu() and will
ultimately hit this KVM_BUG_ON. 

Would it be safer to return an error without the KVM_BUG_ON to prevent
a host denial of service if the system is configured with panic_on_warn?

[ ... ]

> diff --git a/arch/s390/kvm/kvm-s390.c b/arch/s390/kvm/kvm-s390.c
> index 5b2727d7dfd12..21574f57be72e 100644
> --- a/arch/s390/kvm/kvm-s390.c
> +++ b/arch/s390/kvm/kvm-s390.c
> @@ -2934,6 +2934,9 @@ int kvm_arch_vm_ioctl(struct file *filp, unsigned int ioctl, unsigned long arg)
>  	case KVM_S390_INTERRUPT: {
>  		struct kvm_s390_interrupt s390int;
>  
> +		r = -EINVAL;
> +		if (kvm_is_ucontrol(kvm))
> +			break;

[Severity: High]
Does completely fencing the KVM_S390_INTERRUPT ioctl for UCONTROL VMs
cause an ABI regression for older userspace tools?

If existing userspace relies on KVM_S390_INTERRUPT to inject non-external
interrupts (such as KVM_S390_PROGRAM_INT), this unconditional check would
cause those injections to fail with -EINVAL, potentially breaking existing
functionality. 

Could this check be restricted to only rejecting the specific interrupt
types that rely on ESCA?

>  		r = -EFAULT;
>  		if (copy_from_user(&s390int, argp, sizeof(s390int)))
>  			break;

[ ... ]

> @@ -5456,6 +5459,8 @@ long kvm_arch_vcpu_unlocked_ioctl(struct file *filp, unsigned int ioctl,
>  		struct kvm_s390_interrupt s390int;
>  		struct kvm_s390_irq s390irq = {};
>  
> +		if (kvm_is_ucontrol(vcpu->kvm))
> +			return -EINVAL;

[Severity: High]
Is this also prone to the same backwards compatibility regression mentioned
above for vCPU-level interrupt injections?

>  		if (copy_from_user(&s390int, argp, sizeof(s390int)))
>  			return -EFAULT;
>  		if (s390int_to_s390irq(&s390int, &s390irq))

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