Re: [PATCH v3 2/8] 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] Invalid Free / Kernel Panic in kvm_s390_reinject_machine_check() due to kfree() on a stack pointer.
- [High] Stack Information Leak to userspace in kvm_s390_reinject_machine_check() during local machine check reinjection.
--

--- Patch [2]: [PATCH v3 2/8] KVM: s390: Zero initialize data structures for inject_pfault_token ---
commit 99d37a57e11716f103a611788767010736475aa3
Author: Christian Borntraeger <[email protected]>

KVM: s390: Zero initialize data structures for inject_pfault_token

This commit fixes a stack information leak by zero-initializing the
kvm_s390_interrupt and kvm_s390_irq structures in
__kvm_inject_pfault_token. This prevents stale stack values from leaking to
host userspace via migration ioctls.

> 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 = {};
>  	struct kvm_s390_interrupt_info *inti_mem = NULL;

[Severity: Critical]
This isn't a bug introduced by this patch, but while reviewing this fix,
I noticed a similar pre-existing issue in arch/s390/kvm/interrupt.c that
appears to pass a stack pointer to kfree().

In kvm_s390_reinject_machine_check(), a stack-allocated interrupt info
structure is passed to __inject_vm():

kvm_s390_reinject_machine_check() {
    ...
    struct kvm_s390_interrupt_info inti;
    ...
    rc = __inject_vm(vcpu->kvm, &inti);

When the machine check is a floating machine check, this routes to
__inject_float_mchk(), which unconditionally frees the pointer:

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

Could this result in a kernel panic if a nested guest execution triggers
this path, attempting to free a stack-allocated variable?

[Severity: High]
This is also a pre-existing issue, but I noticed another potential stack
information leak in arch/s390/kvm/interrupt.c.

Does kvm_s390_reinject_machine_check() leak uninitialized stack memory
during local machine check reinjection?

kvm_s390_reinject_machine_check() {
    ...
    struct kvm_s390_irq irq;
    ...
    kvm_s390_inject_vcpu(vcpu, &irq);

Because irq is uninitialized on the stack, the padding and fixed_logout
array contain stale data. __inject_mchk() then copies this data into the
persistent vCPU state:

__inject_mchk() {
    ...
    memcpy(&mchk->fixed_logout, &irq->u.mchk.fixed_logout,
           sizeof(mchk->fixed_logout));

Later, when userspace retrieves the interrupt state, store_local_irq()
copies it back out:

store_local_irq() {
    ...
    irq->u.mchk = li->irq.mchk;

Could this allow a host userspace process to access uninitialized kernel
stack memory via the KVM migration ioctls, similar to the issue fixed in
this patch?

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