Re: [PATCH v2 3/7] KVM: s390: Zero initialize irq in reinject_machine_check

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

Pre-existing issues:
- [Critical] Injecting a floating machine check causes a host kernel panic because kfree() is unconditionally called on a stack-allocated variable.
- [High] Memory leak in kvm_s390_get_io_int() when I/O interrupt reinjection fails.
--

--- Patch [3]: [PATCH v2 3/7] KVM: s390: Zero initialize irq in reinject_machine_check ---
commit c87188fae3993586a5800e7440b273a7c1c85398
Author: Christian Borntraeger <[email protected]>
Subject: KVM: s390: Zero initialize irq in reinject_machine_check

This commit zeroes out the stack-allocated kvm_s390_irq structure in
kvm_s390_reinject_machine_check() to prevent leaking uninitialized stack
data via the fixed_logout array.

> diff --git a/arch/s390/kvm/interrupt.c b/arch/s390/kvm/interrupt.c
> index 23f0e0821474a..009d6a845d598 100644
> --- a/arch/s390/kvm/interrupt.c
> +++ b/arch/s390/kvm/interrupt.c
> @@ -3109,7 +3109,7 @@ void kvm_s390_reinject_machine_check(struct kvm_vcpu *vcpu,
>  				     struct mcck_volatile_info *mcck_info)
>  {
>  	struct kvm_s390_interrupt_info inti;

[Severity: Critical]
This is a pre-existing issue, but does this lead to a kernel panic when
injecting a floating machine check?

If a hardware machine check occurs while the CPU is in SIE and it is a
floating machine check, we pass the address of this stack-allocated inti
variable into __inject_vm():

kvm_s390_reinject_machine_check() {
    ...
    if (mci.ck) {
        inti.type = KVM_S390_MCHK;
        rc = __inject_vm(vcpu->kvm, &inti);
    ...
}

Inside __inject_vm(), the call chain reaches __inject_float_mchk(), which
unconditionally frees the pointer:

__inject_float_mchk() {
    ...
    kfree(inti);
}

Could this result in a host crash due to freeing a stack address?

> -	struct kvm_s390_irq irq;
> +	struct kvm_s390_irq irq = {};
>  	struct kvm_s390_mchk_info *mchk;
>  	union mci mci;
>  	__u64 cr14 = 0;         /* upper bits are not used */


While reviewing the interrupt code, I noticed another pre-existing issue in
a different path.

[Severity: High]
This isn't a bug introduced by this patch, but does kvm_s390_get_io_int()
leak memory when I/O interrupt reinjection fails?

When kvm_s390_get_io_int() dequeues a classical I/O interrupt, it takes
ownership of a dynamically allocated struct kvm_s390_interrupt_info object.
If it detects a higher-priority GISA interrupt concurrently, it attempts to
reinject it:

kvm_s390_get_io_int() {
    ...
    kvm_s390_reinject_io_int(kvm, inti);
    ...
}

If the floating interrupt queue is full, __inject_io() returns -EBUSY:

__inject_io() {
    ...
    if (fi->counters[FIRQ_CNTR_IO] >= KVM_S390_MAX_FLOAT_IRQS)
        return -EBUSY;
    ...
}

The return value of the reinjection is ignored in kvm_s390_get_io_int(), and
the original pointer is overwritten immediately after without freeing it on
error, which appears to leak the object.

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