Re: [PATCH v2 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:
- [High] Missing hardware teardown (chsc_sgib(0)) on PCI AEN initialization failure, causing a hardware Use-After-Free.
- [High] Premature free and nullification of `gib` creates a race condition leading to a NULL pointer dereference in the adapter interrupt handler.
--

commit 507ef89f01184c6a51cfce1ef86dd6ded4bac518
Author: Mike Rapoport (Microsoft) <rppt@kernel.org>

KVM: s390: Replace get_zeroed_page() with kzalloc() for the GIB

This patch replaces get_zeroed_page() with kzalloc() for the guest
information block (GIB) allocation, as the size is a page and it does not
strictly require the page allocator directly. It also updates the cleanup
paths to use kfree() instead of free_page().

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;
>  		rc = -EIO;
>  		goto out_unreg_gal;

[Severity: High]
This is a pre-existing issue, but does nullifying gib here before
unregistering the adapter interrupt create a race condition?

If the chsc_sgib() call fails, the error path prematurely frees gib and
sets it to NULL before jumping to out_unreg_gal to unregister the interrupt.
Between setting gib to NULL and the completion of
unregister_adapter_interrupt() (which waits for active handlers via RCU),
the interrupt handler remains active.

If an adapter interrupt fires on the shared ISC during this window,
gib_alert_irq_handler() is invoked and calls process_gib_alert_list().
This function unconditionally executes xchg(&gib->alert_list_origin, ...),
which would dereference the NULL pointer.

[ ... ]

> @@ -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);
>  	gib = NULL;
>  out:
>  	return rc;

[Severity: High]
This isn't a bug introduced by this patch, but does the error path here
forget to detach the GIB from the hardware?

In kvm_s390_gib_init(), if chsc_sgib(gib_origin) successfully registers the
GIB with the hardware, but kvm_s390_pci_aen_init() subsequently fails, the
error path jumps to out_unreg_gal.

This path frees the gib memory but fails to call chsc_sgib(0) to revoke
hardware access (unlike kvm_s390_gib_destroy() which correctly does so).
The hardware retains the physical address of the freed memory and might
continue to access it, resulting in a hardware use-after-free and potential
memory corruption.

-- 
Sashiko AI review · https://sashiko.dev/#/patchset/20260906-s390-kvm-v2-0-2cf6434e6646@kernel.org?part=3