Re: [PATCH v5 1/4] s390/vfio-ap: Fix leak of pinned NIB and registered NISC in vfio_ap_irq_enable/disable()

[sashiko-bot@kernel.org]

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

New issues:
- [High] Unbounded memory leak of newly pinned NIB pages and GISC registrations when `vfio_ap_irq_enable` times out.

Pre-existing issues:
- [High] Guest-triggerable WARN_ONCE introduces a host-side denial of service risk.
--

commit 82e69024e1f94318c978aa31bed7a9d1888b87e2
Author: Anthony Krowiak <akrowiak@linux.ibm.com>

s390/vfio-ap: Fix leak of pinned NIB and registered NISC in vfio_ap_irq_enable/disable()

This commit modifies the cleanup of pinned NIBs and registered NISCs after
PQAP(AQIC) instructions are executed. It introduces a wait loop to verify the
asynchronous completion of the interrupt state transition before allowing
the release of older resources.

> diff --git a/drivers/s390/crypto/vfio_ap_ops.c b/drivers/s390/crypto/vfio_ap_ops.c
> index 940c0ff668bee..8fb0476e3d397 100644
> --- a/drivers/s390/crypto/vfio_ap_ops.c
> +++ b/drivers/s390/crypto/vfio_ap_ops.c
[ ... ]
> +static bool vfio_ap_wait_for_irqstate(int apqn, int ir)
>  {
>  	struct ap_queue_status status;
>  	int retry = 5;
[ ... ]
> -	WARN_ONCE(1, "%s: tapq rc %02x: %04x could not clear IR bit\n",
> -		  __func__, status.response_code, apqn);
> +	WARN_ONCE(1, "%s: tapq rc %02x: timed out waiting for interrupts %s for %02x.%04x\n",
> +		  __func__, status.response_code,
> +		  ir ? "enabled" : "disabled",
> +		  AP_QID_CARD(apqn), AP_QID_QUEUE(apqn));
> +	return false;
>  }

[Severity: High]
This is a pre-existing issue, but does this WARN_ONCE introduce a guest-driven
host denial of service risk?

If a guest issues a PQAP(AQIC) instruction and the hardware takes longer than
expected to transition states, this wait loop times out and triggers a warning.
Because the KVM subsystem strictly forbids guest-reachable WARN_ON macros,
could a malicious guest artificially induce these delays (for instance, by
spamming requests) to intentionally panic the host kernel if panic_on_warn
is enabled?

[ ... ]
> @@ -317,8 +334,30 @@ static struct ap_queue_status vfio_ap_irq_disable(struct vfio_ap_queue *q)
>  		switch (status.response_code) {
[ ... ]
>  		case AP_RESPONSE_INVALID_ADDRESS:
>  		default:
> -			/* All cases in default means AP not operational */
> +			/*
> +			 * The AQIC disable was rejected; IRQ is still enabled
> +			 * and the hardware still holds the NIB address. Do not
> +			 * free resources.
> +			 */
>  			WARN_ONCE(1, "%s: ap_aqic status %d\n", __func__,
>  				  status.response_code);
> -			goto end_free;
> +			goto end_fail;
>  		}
>  	} while (retries--);

[Severity: High]
This is a pre-existing issue, but does this WARN_ONCE pose the same guest-
driven denial of service risk?

If the hardware returns an unexpected status code in response to a guest-
initiated disable request, this warning is hit. Can an untrusted guest
intentionally induce unexpected hardware states to trigger this warning and
crash the host?

[ ... ]
> @@ -489,13 +557,49 @@ static struct ap_queue_status vfio_ap_irq_enable(struct vfio_ap_queue *q,
>  	status = ap_aqic(q->apqn, aqic_gisa, h_nib);
>  	switch (status.response_code) {
>  	case AP_RESPONSE_NORMAL:
> -		/* See if we did clear older IRQ configuration */
> +		/*
> +		 * AQIC initiates an asynchronous process; however, AP_RESPONSE_NORMAL
> +		 * does not guarantee interrupts are enabled yet (i.e., IR bit (7)
> +		 * is set). Wait to confirm before committing the new NIB and
> +		 * freeing the old resources.
> +		 */
> +		if (!vfio_ap_wait_for_irqstate(q->apqn, AP_IRQ_ENABLED)) {
> +			/*
> +			 * Timed out waiting to verify IRQs are enabled. If the
> +			 * hardware is merely stalled, it might eventually complete
> +			 * and write interrupt status bytes to the new NIB.
> +			 *
> +			 * If the NIB page is unpinned and freed here, this delayed
> +			 * hardware write would result in a host use-after-free/wild
> +			 * DMA write and a host kernel crash.
> +			 *
> +			 * To prevent this, we must leak the new resources (leave the
> +			 * NIB page pinned and Guest ISC registered) and return
> +			 * AP_RESPONSE_OTHERWISE_CHANGED to signal the guest to retry.
> +			 */
> +			msg = "%s: Timed out waiting to verify IRQs enabled for apqn=%#04x\n";
> +			VFIO_AP_DBF_WARN(msg, __func__, q->apqn);
> +			memset(&status, 0, sizeof(status));
> +			status.response_code = AP_RESPONSE_OTHERWISE_CHANGED;
> +			break;
> +		}

[Severity: High]
Will this timeout path result in an unbounded memory leak?

When vfio_ap_wait_for_irqstate() times out, the code breaks out of the switch
statement without storing the newly pinned 'nib' and registered 'isc' into
q->saved_iova or q->saved_isc. These local variables appear to be permanently
orphaned.

Because a guest controls the PQAP calls, could an untrusted guest VM repeatedly
trigger this timeout to deliberately leak pinned host pages and KVM GISC
registrations until host memory is exhausted?

Additionally, this seems to contradict the commit message which states:
"The fix is to unregister the NISC and an unpin the NIB used in the AQIC
call in the default case of the switch statement..."
Here the code explicitly notes "we must leak the new resources". Is this
omission of cleanup intentional?

-- 
Sashiko AI review · https://sashiko.dev/#/patchset/20260831171443.222225-1-akrowiak@linux.ibm.com?part=1