Re: [PATCH 2/3] KVM: s390: zero initialize irq 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 pointer during floating machine check reinjection.
- [High] Uninitialized kernel stack memory in the `fixed_logout` array is leaked to host userspace via the KVM_GET_IRQ_STATE ioctl.
--

--- Patch [2]: [PATCH 2/3] KVM: s390: zero initialize irq for inject_pfault_token ---
commit f6de66eb1335bc1e8dc54d73ed3ad8d7b39ac810
Author: Christian Borntraeger <[email protected]>

KVM: s390: zero initialize irq for inject_pfault_token

This commit fixes a potential stack information leak to userspace via
migration ioctls. It zero-initializes the irq structure in
__kvm_inject_pfault_token() to ensure stale stack values are not copied.

> diff --git a/arch/s390/kvm/kvm-s390.c b/arch/s390/kvm/kvm-s390.c
> index 518a69c55e85c..4f4ac6a8d1ad3 100644
> --- a/arch/s390/kvm/kvm-s390.c
> +++ b/arch/s390/kvm/kvm-s390.c
> @@ -4475,7 +4475,7 @@ 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_irq irq = {};

[Severity: High]
This isn't a bug introduced by this patch, but while looking at stack memory 
leaks, does a similar uninitialized stack leak happen in 
kvm_s390_reinject_machine_check() in arch/s390/kvm/interrupt.c?

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

If the machine check is injected into the vCPU (mci.ck is false), several
fields are populated, but the fixed_logout array might be left
uninitialized. 

When passed to kvm_s390_inject_vcpu() -> __inject_mchk(), the uninitialized
fixed_logout array is copied into the vCPU's pending interrupt state:

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

Later, if a host user calls the KVM_S390_GET_IRQ_STATE ioctl, this state
could be copied to userspace:

arch/s390/kvm/interrupt.c:kvm_s390_get_irq_state() {
    ...
    if (copy_to_user(&buf[n], &irq, sizeof(irq)))
    ...
}

Could this leak kernel stack memory in the fixed_logout array to userspace?


[Severity: Critical]
This is another pre-existing issue, but while looking at 
kvm_s390_reinject_machine_check(), can a stack pointer be incorrectly freed?

It allocates struct kvm_s390_interrupt_info inti on the kernel stack:

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

If mci.ck is true, it attempts to inject a floating machine check by passing
the stack-allocated struct by reference:

arch/s390/kvm/interrupt.c:kvm_s390_reinject_machine_check() {
    ...
    rc = __inject_vm(vcpu->kvm, &inti);
    ...
}

The injection routes KVM_S390_MCHK to __inject_float_mchk(), which
unconditionally calls kfree() on the passed pointer:

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

Would calling kfree() on this stack pointer cause a kernel panic?

>  	struct kvm_s390_interrupt_info *inti_mem = NULL;
>  	int ret = 0;

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