Re: [PATCH 3/4] KVM: s390: Replace get_zeroed_page() with kzalloc() for the GIB

[sashiko-bot@kernel.org]

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

Pre-existing issues:
- [Critical] Hardware Use-After-Free due to missing `chsc_sgib(0)` on `kvm_s390_pci_aen_init` failure path.
- [High] Race condition in `kvm_s390_gib_init()` error path leading to a NULL pointer dereference.
--

commit b7e0a2c7fc946fc151e2d875aa6df00a71151750
Author: Mike Rapoport (Microsoft) <rppt@kernel.org>
Subject: KVM: s390: Replace get_zeroed_page() with kzalloc() for the GIB

This commit replaces the use of get_zeroed_page() with kzalloc() and
free_page() with kfree() for the guest information block (GIB) allocation.
It notes that kmalloc() guarantees page alignment for power-of-two sizes,
making it suitable for this buffer without requiring direct page allocator
access.

Link: https://lore.kernel.org/all/635405e4-9423-4a25-a6e7-e03c8ea0bcbe@redhat.com

> diff --git a/arch/s390/kvm/s390/interrupt.c b/arch/s390/kvm/s390/interrupt.c
> index 0381ae9817035..02065fb8d053e 100644
> --- a/arch/s390/kvm/s390/interrupt.c
> +++ b/arch/s390/kvm/s390/interrupt.c

[ ... ]

> @@ -3722,7 +3722,7 @@ int __init kvm_s390_gib_init(u8 nisc)
>  	gib_origin = virt_to_phys(gib);
>  	if (chsc_sgib(gib_origin)) {
>  		pr_err("Associating the GIB with the AIV facility failed\n");
> -		free_page((unsigned long)gib);
> +		kfree(gib);
>  		gib = NULL;

[Severity: High]
This is a pre-existing issue, but I noticed a potential race condition in
this error path in kvm_s390_gib_init().

When chsc_sgib() fails, the code frees gib and sets it to NULL. However,
this happens before calling unregister_adapter_interrupt(&gib_alert_irq) at
the out_unreg_gal label.

Since the interrupt handler is still registered and active, if another device
sharing the same ISC triggers an interrupt during this window, could
the gib_alert_irq_handler execute and call process_gib_alert_list()? 
If it does, it looks like it will unconditionally dereference the NULL gib
pointer.

>  		rc = -EIO;
>  		goto out_unreg_gal;

[ ... ]

> @@ -3742,7 +3742,7 @@ int __init kvm_s390_gib_init(u8 nisc)
>  out_unreg_gal:
>  	unregister_adapter_interrupt(&gib_alert_irq);
>  out_free_gib:
> -	free_page((unsigned long)gib);
> +	kfree(gib);

[Severity: Critical]
This is also a pre-existing issue, but does this cleanup path result in a
hardware use-after-free if kvm_s390_pci_aen_init() fails?

In kvm_s390_gib_init(), if the GIB is successfully registered with the
hardware via chsc_sgib(gib_origin), but the subsequent call to
kvm_s390_pci_aen_init() fails, the code jumps to out_unreg_gal:

	if (kvm_s390_pci_interp_allowed()) {
		if (kvm_s390_pci_aen_init(nisc)) {
			pr_err("Initializing AEN for PCI failed\n");
			rc = -EIO;
			goto out_unreg_gal;
		}
	}

This skips calling chsc_sgib(0) to tear down the hardware registration.
When we reach out_free_gib and free the memory backing the GIB, doesn't
the hardware AIV facility still retain the physical pointer to the freed
memory?

>  	gib = NULL;
>  out:
>  	return rc;

-- 
Sashiko AI review · https://sashiko.dev/#/patchset/20260902-s390-kvm-v1-0-3bc0986550b1@kernel.org?part=3