Re: [PATCH v2] KVM: s390: Improve floating IRQ injection behavior

Thank you for your contribution! Sashiko AI review found 2 potential issue(s) to consider:
- [Critical] Use-After-Free of `inti` when injecting floating interrupts.
- [High] Missed vCPU kick causes lost or delayed floating interrupts when all vCPUs have masks closed.
--

--- Patch [1]: [PATCH v2] KVM: s390: Improve floating IRQ injection behavior ---
commit e7cb031c2f46e8290e932cd848faad5eec943d11
Author: Janosch Frank <[email protected]>
Subject: KVM: s390: Improve floating IRQ injection behavior

Floating IRQs can be handled by any VCPU that opened its masks. The
current design does not check if the mask is open when a floating IRQ
is injected via the FLIC. It will wakeup the last VCPU that went
sleeping hoping it's the correct one.

> diff --git a/arch/s390/kvm/interrupt.c b/arch/s390/kvm/interrupt.c
> index 8f24bcd1a6d36..e67cdedb5aece 100644
> --- a/arch/s390/kvm/interrupt.c
> +++ b/arch/s390/kvm/interrupt.c

[ ... ]

> @@ -1915,22 +1946,48 @@ static int __inject_io(struct kvm *kvm, struct kvm_s390_interrupt_info *inti)

[ ... ]

>  /*
>   * Find a destination VCPU for a floating irq and kick it.
>   */
> -static void __floating_irq_kick(struct kvm *kvm, u64 type)
> +static void __floating_irq_kick(struct kvm *kvm, struct kvm_s390_interrupt_info *inti)
>  {
>  	struct kvm_vcpu *dst_vcpu;
>  	int sigcpu, online_vcpus, nr_tries = 0;
> +	u64 type = READ_ONCE(inti->type);
> +	u64 irq_pend_mask;
>  
>  	online_vcpus = atomic_read(&kvm->online_vcpus);
>  	if (!online_vcpus)
>  		return;
>  
> +	irq_pend_mask = inti_to_irq_pend_mask(inti);
>  	for (sigcpu = kvm->arch.float_int.last_sleep_cpu; ; sigcpu++) {
>  		sigcpu %= online_vcpus;
>  		dst_vcpu = kvm_get_vcpu(kvm, sigcpu);
> -		if (!is_vcpu_stopped(dst_vcpu))
> +		if (!is_vcpu_stopped(dst_vcpu) &&
> +		    deliverable_irqs(dst_vcpu) & irq_pend_mask)
>  			break;
>  		/* avoid endless loops if all vcpus are stopped */
>  		if (nr_tries++ >= online_vcpus)
>  			return;

[Severity: High]
Does this code drop the interrupt kick entirely if all vCPUs currently
have their interrupt masks closed?

If deliverable_irqs(dst_vcpu) & irq_pend_mask evaluates to 0 for all running
vCPUs, this loop will exit via the nr_tries check and return early.

Because it returns early, kvm_s390_set_cpuflags() and kvm_s390_vcpu_wakeup()
are never executed, meaning no vCPU drops out of SIE and
set_intercept_indicators() is not called. 

If the guest later unmasks the interrupt, could it remain undelivered since
the hardware intercept was never set up?

[ ... ]

> @@ -1982,7 +2039,7 @@ static int __inject_vm(struct kvm *kvm, struct kvm_s390_interrupt_info *inti)
>  	if (rc)
>  		return rc;
>  
> -	__floating_irq_kick(kvm, type);
> +	__floating_irq_kick(kvm, inti);

[Severity: Critical]
Does this cause a use-after-free when injecting GISA I/O adapter interrupts?

When __inject_io() handles a GISA I/O interrupt, it frees the interrupt info:

arch/s390/kvm/interrupt.c:__inject_io() {
    ...
    if (gi->origin && inti->type & KVM_S390_INT_IO_AI_MASK) {
        ...
        kfree(inti);
        return 0;
    }
    ...
}

Since it returns 0, __inject_vm() will proceed to call __floating_irq_kick()
with the newly freed inti pointer, which then dereferences it when calling
inti_to_irq_pend_mask(inti).

Also, for non-GISA interrupts, inti is placed on a shared list where another
vCPU might deliver and free it before __floating_irq_kick() runs. 

Should we avoid passing inti to the kick function?

>  	return 0;
>  }

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