Re: [PATCH v4 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:
- [Critical] Prematurely unpinning the new NIB page on a hardware timeout in `vfio_ap_irq_enable` causes a host use-after-free and wild DMA write if the hardware later completes the asynchronous interrupt configuration.

Pre-existing issues:
- [High] Guest-reachable `WARN_ONCE` in `vfio_ap_wait_for_irqstate` and `vfio_ap_irq_disable` violates KVM subsystem guidelines and creates a denial of service vector on `panic_on_warn` systems.
--

commit f514ef27dfc693d5c326ffed4fd1a9985acce1d0
Author: Anthony Krowiak <akrowiak@linux.ibm.com>
Subject: s390/vfio-ap: Fix leak of pinned NIB and registered NISC in vfio_ap_irq_enable/disable()

This commit addresses resource leaks by restructuring how the AQIC
instruction responses are handled for AP queues. It introduces a polling
mechanism to wait and confirm asynchronous hardware transitions before freeing
resources.

> diff --git a/drivers/s390/crypto/vfio_ap_ops.c b/drivers/s390/crypto/vfio_ap_ops.c
> index 940c0ff668bee..24e93fb7f81a2 100644
> --- a/drivers/s390/crypto/vfio_ap_ops.c
> +++ b/drivers/s390/crypto/vfio_ap_ops.c

[ ... ]

> @@ -245,8 +259,8 @@ static void vfio_ap_wait_for_irqclear(int apqn)
>  		switch (status.response_code) {
>  		case AP_RESPONSE_NORMAL:
>  		case AP_RESPONSE_RESET_IN_PROGRESS:
> -			if (!status.irq_enabled)
> -				return;
> +			if (status.irq_enabled == ir)
> +				return true;
>  			fallthrough;
>  		case AP_RESPONSE_BUSY:
>  			msleep(20);
> @@ -257,12 +271,15 @@ static void vfio_ap_wait_for_irqclear(int apqn)
>  		default:
>  			WARN_ONCE(1, "%s: tapq rc %02x: %04x\n", __func__,
>  				  status.response_code, apqn);
> -			return;
> +			return true;
>  		}
>  	} while (--retry);
>  
> -	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 isn't a bug introduced by this patch, but does this guest-reachable
warning violate KVM subsystem guidelines?

A malicious guest can intentionally manipulate AP queue states or rapidly
spam PQAP instructions to stall the hardware, triggering these warnings on
non-operational states or polling timeouts. On systems configured with
panic_on_warn, this creates a host denial of service vector.

Could these be converted to rate-limited debug messages or dropped entirely?

[ ... ]

> @@ -326,18 +365,46 @@ static struct ap_queue_status vfio_ap_irq_disable(struct vfio_ap_queue *q)
>  		case AP_RESPONSE_Q_NOT_AVAIL:
>  		case AP_RESPONSE_DECONFIGURED:
>  		case AP_RESPONSE_CHECKSTOPPED:
> +			/* AP not operational; no further interrupts possible */
> +			WARN_ONCE(1, "%s: ap_aqic status %d\n", __func__,
> +				  status.response_code);
> +			goto end_free;
>  		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;
>  		}

[Severity: High]
This is also a pre-existing issue, but can this similarly be triggered by 
an unprivileged guest manipulating the queue into an unavailable state during
a disable attempt?

[ ... ]

> @@ -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 the IR bit is set yet.  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: the hardware may not have accepted the new
> +			 * NIB.  Clean up the new resources and return
> +			 * OTHERWISE_CHANGED to signal the guest to retry.
> +			 */
> +			ret = kvm_s390_gisc_unregister(kvm, isc);
> +			if (ret) {
> +				msg = "%s: kvm_s390_gisc_unregister: rc=%d isc=%d, apqn=%#04x\n";
> +				VFIO_AP_DBF_WARN(msg, __func__, ret, isc, q->apqn);
> +			}
> +			vfio_unpin_pages(&q->matrix_mdev->vdev, nib, 1);
> +			memset(&status, 0, sizeof(status));
> +			status.response_code = AP_RESPONSE_OTHERWISE_CHANGED;
> +			break;
> +		}

[Severity: Critical]
Does unpinning the new NIB page here introduce a race that could lead to a
wild DMA write and host memory corruption?

Because ap_aqic() returned AP_RESPONSE_NORMAL, the hardware accepted the
asynchronous request. If the hardware is merely stalled (e.g., due to a
malicious guest spamming instructions) and vfio_ap_wait_for_irqstate() times
out, the hardware might still eventually complete the transition and write
interrupt status bytes to the new NIB.

If vfio_unpin_pages() is called here, that physical page could be reallocated
by the host kernel for a different purpose before the delayed hardware DMA
write completes.

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