Re: [PATCH v2 2/7] KVM: s390: Zero initialize data structures for inject_pfault_token

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

Pre-existing issues:
- [Critical] Calling kfree() on a stack-allocated variable when reinjecting floating machine checks leads to a guest-triggerable kernel panic.
- [High] Uninitialized stack memory in `struct kvm_s390_irq` is leaked to the guest and userspace during machine check reinjection.
--

--- Patch [2]: [PATCH v2 2/7] KVM: s390: Zero initialize data structures for inject_pfault_token ---
commit 2b81aa54a540c9ab3f152dc0b18d8b86e716252d
Author: Christian Borntraeger <[email protected]>

KVM: s390: Zero initialize data structures for inject_pfault_token

This commit fixes an issue where __kvm_inject_pfault_token() leaves fields
in the on-stack struct kvm_s390_irq uninitialized. These stale stack values
are copied into the cpu local variable, allowing a host user to access them
via migration ioctls. The fix zero-initializes the irq and inti structures.

> diff --git a/arch/s390/kvm/kvm-s390.c b/arch/s390/kvm/kvm-s390.c
> index 518a69c55e85c..611dcb5342101 100644
> --- a/arch/s390/kvm/kvm-s390.c
> +++ b/arch/s390/kvm/kvm-s390.c
> @@ -4474,8 +4474,8 @@ int kvm_s390_try_set_tod_clock(struct kvm *kvm, const struct kvm_s390_vm_tod_clo
>  static void __kvm_inject_pfault_token(struct kvm_vcpu *vcpu, bool start_token,
>  				     unsigned long token)
>  {
> -	struct kvm_s390_interrupt inti;
> -	struct kvm_s390_irq irq;
> +	struct kvm_s390_interrupt inti = {};
> +	struct kvm_s390_irq irq = {};

[Severity: Critical]
This is a pre-existing issue, but while looking at these stack variables,
I noticed a similar pattern in arch/s390/kvm/interrupt.c that might lead to a
kernel panic.

In kvm_s390_reinject_machine_check(), the interrupt structure is allocated on
the kernel stack:

arch/s390/kvm/interrupt.c:kvm_s390_reinject_machine_check() {
    ...
    struct kvm_s390_interrupt_info inti;
    ...
}

If a malicious L1 guest crafts a nested vSIE control block with mcic set to
include the mci.ck bit, __inject_vm() delegates to __inject_float_mchk(),
which then unconditionally frees the stack pointer:

arch/s390/kvm/interrupt.c:__inject_float_mchk() {
    ...
    kfree(inti);
    ...
}

Could this allow an untrusted, unprivileged guest to trigger an immediate
kernel panic by corrupting the SLUB allocator?

[Severity: High]
This is a pre-existing issue, but there appears to be another uninitialized
stack leak in arch/s390/kvm/interrupt.c.

Also in kvm_s390_reinject_machine_check(), the irq structure is allocated on
the kernel stack without zero-initialization:

arch/s390/kvm/interrupt.c:kvm_s390_reinject_machine_check() {
    ...
    struct kvm_s390_irq irq;
    ...
}

If triggered via vSIE without the mci.ck bit, the fixed_logout field remains
uninitialized. When __inject_mchk() is called, it blindly copies this memory:

arch/s390/kvm/interrupt.c:__inject_mchk() {
    ...
    memcpy(&mchk->fixed_logout, &irq->u.mchk.fixed_logout,
           sizeof(mchk->fixed_logout));
    ...
}

This uninitialized host kernel stack data is then written directly to guest
memory via __write_machine_check(), or leaked via KVM_GET_IRQ_STATE:

arch/s390/kvm/interrupt.c:__write_machine_check() {
    ...
    rc |= write_guest_lc(vcpu, __LC_PSW_SAVE_AREA, &mchk->fixed_logout,
                         sizeof(mchk->fixed_logout));
    ...
}

Could this allow an untrusted guest to repeatedly dump 16 bytes of
uninitialized host kernel stack memory, potentially bypassing KASLR or leaking
sensitive information?

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