[PATCH v5 0/4] s390/vfio-ap: Fix pre-existing bugs in vfio_ap device driver

[PATCH v5 0/4] s390/vfio-ap: Fix pre-existing bugs in vfio_ap device driver

[Anthony Krowiak]

The sashiko AI reported several pre-existing bugs in the vfio_ap device
driver code while reviewing unrelated patches. This series fixes four
such bugs.

Change log v4 => v5:
~~~~~~~~~~~~~~~~~~~
Patch 1/4: Fix leak of pinned NIB and registered NISC in vfio_ap_irq_enable
* Fixed bug in vfio_ap_irq_enable() whereby the AQIC resources are freed
  when vfio_ap_wait_for_irq_state() returns false.

Patch 3/4: Fix unbounded loop in apq_reset_check()
* Refactored apq_status_check() to verify the queue was zeroized.

Anthony Krowiak (4):
  s390/vfio-ap: Fix leak of pinned NIB and registered NISC in
    vfio_ap_irq_enable/disable()
  s390/vfio-ap: Fix failure to release IRQ notification eventfd contexts
  s390/vfio-ap: Fix unbounded loop in apq_reset_check()
  s390/vfio-ap: Use AP_DOMAINS for adm_add bitmap size in
    vfio_ap_mdev_cfg_add()

 drivers/s390/crypto/vfio_ap_ops.c | 273 ++++++++++++++++++++++++++----
 1 file changed, 243 insertions(+), 30 deletions(-)

-- 
2.53.0

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

[Anthony Krowiak]

The vfio_ap_irq_enable() and vfio_ap_disable() functions execute the
PQAP(AQIC) instructions to enable/disable interrupts for an AP queue.
A switch statement is used to examine the status response code returned
from the instruction to determine whether it succeeded or failed and react
accordingly.

vfio_ap_irq_enable()
~~~~~~~~~~~~~~~~~~~~
For the default case, the vfio_ap_irq_disable function is invoked to
disable interrupts for the queue and clean up the AQIC resources (i.e.,
unpin the NIB and unregister the NISC) that are stored with the
vfio_ap_queue object. There are a number of problems with this:

1. Neither the q->saved_iova nor q->saved_isc has been set for the current
   AQIC call, so the AQIC resources - assuming those values have been
   previously set - will be the NIB and NISC resources from a prior call;
   the NIB and NISC from the current call are therefore leaked.

2. Interrupts may never have been enabled. Sending a disable instruction to
   a queue that the hardware just told you is in a bad state (CHECKSTOPPED,
   DECONFIGURED, Q_NOT_AVAIL) is at best wasted work and at worst generates
   a further WARN_ONCE from inside vfio_ap_irq_disable's own default.

3. The hardware just rejected the new ap_aqic() enable attempt with an
   unexpected status. Disabling a previously-working IRQ config - assuming
   that is even possible - as a reaction to a failed enable attempt does
   not make sense; it is actively destructive, tearing down something that
   was working for no valid reason.

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 and leave the AQIC
resources stored with the vfio_ap_queue object alone.

Another problem with vfio_ap_irq_enable() is that the old NIB stored in
q->saved_iova from a previous AQIC are immediately freed - assuming they
are stored - as soon as the AQIC response status returns
AP_RESPONSE_NORMAL. The problem with this is, AQIC is an asynchronous
operation; the only way to tell if it has completed is to check the
I-bit (7) in the status returned from AQIC. This bit indicates whether
interrupts are enabled (1) or disabled (0).

The fix for this is to wait a specified period of time until the I-bit is
set, similar to vfio_ap_wait_for_irqclear() - waits for I-bit==0 - which
is called from vfio_ap_irq_disable() to verify the operation has completed.
If verification of I-bit == 1 occurs within a specified period of time,
freeing the old NIB (q->saved_iova) because once the I-bit is set, the new
NIB is made available for queue interrupts and any old NIB will no longer
be used. If the verification times out, then the new NIB will
be freed and the old NIB will be allowed to leak which is preferable
because the queue might still have in-flight DMA writes directed to the
old NIB which would result in a use-after-free kernel crash.

vfio_ap_irq_disable()
~~~~~~~~~~~~~~~~~~~~~
There are two problems with the way this function handles the response
code returned from the PQAP(AQIC) instruction:

1. For response codes AP_RESPONSE_NORMAL or AP_RESPONSE_OTHERWISE_CHANGED,
   a call is made to vfio_ap_wait_for_irqclear() which waits for the
   I-bit (7) - indicates whether interrupts are enabled (1) or
   disabled (0) - to be cleared. That function does not return anything, so
   there is no way to determine whether it succeeded or not. The
   vfio_ap_irq_disable() function then frees the AQIC resources. This is a
   problem because the hardware may still write to the NIB resulting in a
   use-after-free kernel crash.

   The fix for this is to add a boolean return code from
   vfio_ap_wait_for_irqclear(). This will be checked in
   vfio_ap_irq_disable() and if clearing of the IR bit could not be
   verified, the AQIC resources will be allowed to leak. This is
   preferable to a kernel crash.

2. For response code AP_RESPONSE_INVALID_ADDRESS - indicates the NIB
   address passed to PQAP(AQIC) is not valid - as well as the default case,
   the vfio_ap_irq_disable() frees the AQIC resources. Since the AQIC
   disable was rejected, the IRQ is still enabled and the hardware still
   holds the NIB address, so freeing the NIB could result in a
   use-after-free kernel crash.

   The fix for this is to allow the AQIC resources to be leaked. This is
   preferable to a kernel crash.

Fixes: ec89b55e3bce7 ("s390: ap: implement PAPQ AQIC interception in kernel")
Cc: stable@vger.kernel.org
Signed-off-by: Anthony Krowiak <akrowiak@linux.ibm.com>
---
 drivers/s390/crypto/vfio_ap_ops.c | 152 +++++++++++++++++++++++++-----
 1 file changed, 129 insertions(+), 23 deletions(-)

diff --git a/drivers/s390/crypto/vfio_ap_ops.c b/drivers/s390/crypto/vfio_ap_ops.c
index 940c0ff668be..8fb0476e3d39 100644
--- a/drivers/s390/crypto/vfio_ap_ops.c
+++ b/drivers/s390/crypto/vfio_ap_ops.c
@@ -30,6 +30,9 @@
 #define AP_QUEUE_UNASSIGNED "unassigned"
 #define AP_QUEUE_IN_USE "in use"
 
+#define AP_IRQ_DISABLED	0
+#define AP_IRQ_ENABLED	1
+
 #define AP_RESET_INTERVAL		20	/* Reset sleep interval (20ms)		*/
 
 static int vfio_ap_mdev_reset_queues(struct ap_matrix_mdev *matrix_mdev);
@@ -226,16 +229,27 @@ static struct vfio_ap_queue *vfio_ap_mdev_get_queue(
 }
 
 /**
- * vfio_ap_wait_for_irqclear - clears the IR bit or gives up after 5 tries
- * @apqn: The AP Queue number
+ * vfio_ap_wait_for_irqstate - wait for the IR bit to reach the requested state
  *
- * Checks the IRQ bit for the status of this APQN using ap_tapq.
- * Returns if the ap_tapq function succeeded and the bit is clear.
- * Returns if ap_tapq function failed with invalid, deconfigured or
- * checkstopped AP.
- * Otherwise retries up to 5 times after waiting 20ms.
+ * @apqn: the APQN of the queue
+ * @ir: the expected state of the IR bit: AP_IRQ_DISABLED or AP_IRQ_ENABLED
+ *
+ * Repeatedly polls the AP queue status via PQAP(TAPQ) every 20ms until the IR
+ * bit matches @ir, the queue becomes non-operational, or 5 retries are
+ * exhausted.
+ *
+ * Because PQAP(AQIC) initiates an asynchronous process, a condition-code 0
+ * completion does not guarantee the IR bit has reached the requested state.
+ * The caller must use this function to confirm the state before proceeding.
+ *
+ * Return:
+ * - true if the IR bit matches @ir, or the AP is non-operational (in which
+ *   case no further interrupts can be generated)
+ *
+ * - false if the IR bit still does not match @ir after all retries are
+ *   exhausted
  */
-static void vfio_ap_wait_for_irqclear(int apqn)
+static bool vfio_ap_wait_for_irqstate(int apqn, int ir)
 {
 	struct ap_queue_status status;
 	int retry = 5;
@@ -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;
 }
 
 /**
@@ -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_OTHERWISE_CHANGED:
 		case AP_RESPONSE_NORMAL:
-			vfio_ap_wait_for_irqclear(q->apqn);
-			goto end_free;
+			/*
+			 * AQIC disable was accepted (NORMAL), or the queue was
+			 * already disabled or a prior async request is still
+			 * completing (OTHERWISE_CHANGED).  In both cases, we must
+			 * wait until interrupt processing has been disabled
+			 * before proceeding.
+			 */
+			if (vfio_ap_wait_for_irqstate(q->apqn, AP_IRQ_DISABLED))
+				goto end_free;
+			/*
+			 * Timed out waiting to confirm interrupts are disabled.
+			 * If ap_aqic returned NORMAL, the guest would incorrectly
+			 * interpret that as a successful disable and may free or
+			 * reuse the NIB while hardware can still write to it.
+			 * Zero the status word and set OTHERWISE_CHANGED to mimic
+			 * what the hardware does for that response code. This
+			 * signals to the guest that the reset operation did not
+			 * complete.
+			 */
+			if (status.response_code == AP_RESPONSE_NORMAL) {
+				memset(&status, 0, sizeof(status));
+				status.response_code = AP_RESPONSE_OTHERWISE_CHANGED;
+			}
+			goto end_fail;
 		case AP_RESPONSE_RESET_IN_PROGRESS:
 		case AP_RESPONSE_BUSY:
 			msleep(20);
@@ -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;
 		}
 	} while (retries--);
 
 	WARN_ONCE(1, "%s: ap_aqic status %d\n", __func__,
 		  status.response_code);
+
+end_fail:
+	/*
+	 * We are here either because of a failure to verify that
+	 * interrupts have been disabled, or because the AQIC instruction
+	 * failed to disable them. The AQIC resources - the pinned NIB page
+	 * and the registered guest ISC - cannot be freed here. The hardware
+	 * may still write to the NIB; freeing the pinned page would result
+	 * in a use-after-free kernel crash. The resources will therefore be
+	 * leaked. This is preferable to a use-after-free.
+	 */
+	return status;
+
 end_free:
+	/*
+	 * This label is reached because the queue was successfully disabled,
+	 * or because the queue is not operational, in which case interrupts
+	 * can not be processed, so free the AQIC resources - the pinned NIB
+	 * page and the registered guest ISC - used to enable interrupts
+	 * so they will not be leaked.
+	 */
 	vfio_ap_free_aqic_resources(q);
 	return status;
 }
@@ -432,6 +499,7 @@ static struct ap_queue_status vfio_ap_irq_enable(struct vfio_ap_queue *q,
 	struct kvm *kvm;
 	phys_addr_t h_nib;
 	dma_addr_t nib;
+	char *msg;
 	int ret;
 
 	/* Verify that the notification indicator byte address is valid */
@@ -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;
+		}
+		/*
+		 * Now that IR=1 is confirmed (IRQs enabled), the
+		 * new NIB is in use for this queue, so no interrupts can be made
+		 * pending via any previously-registered NIB and the old
+		 * resources can be safely freed.
+		 */
 		vfio_ap_free_aqic_resources(q);
 		q->saved_iova = nib;
 		q->saved_isc = isc;
 		break;
 	case AP_RESPONSE_OTHERWISE_CHANGED:
-		/* We could not modify IRQ settings: clear new configuration */
+		/*
+		 * IRQ control is already set as requested or a prior async
+		 * request has not yet completed; in either case, this response
+		 * comes with CC=3 indicating the new NIB and ISC were not accepted by
+		 * the hardware, so clean them up.
+		 */
 		ret = kvm_s390_gisc_unregister(kvm, isc);
 		if (ret)
 			VFIO_AP_DBF_WARN("%s: kvm_s390_gisc_unregister: rc=%d isc=%d, apqn=%#04x\n",
@@ -503,9 +607,12 @@ static struct ap_queue_status vfio_ap_irq_enable(struct vfio_ap_queue *q,
 		vfio_unpin_pages(&q->matrix_mdev->vdev, nib, 1);
 		break;
 	default:
-		pr_warn("%s: apqn %04x: response: %02x\n", __func__, q->apqn,
-			status.response_code);
-		vfio_ap_irq_disable(q);
+		/* We could not modify IRQ settings: clear new configuration */
+		ret = kvm_s390_gisc_unregister(kvm, isc);
+		if (ret)
+			VFIO_AP_DBF_WARN("%s: kvm_s390_gisc_unregister: rc=%d isc=%d, apqn=%#04x\n",
+					 __func__, ret, isc, q->apqn);
+		vfio_unpin_pages(&q->matrix_mdev->vdev, nib, 1);
 		break;
 	}
 
@@ -635,7 +742,6 @@ static int handle_pqap(struct kvm_vcpu *vcpu)
 	}
 
 	status = vcpu->run->s.regs.gprs[1];
-
 	/* If IR bit(16) is set we enable the interrupt */
 	if ((status >> (63 - 16)) & 0x01)
 		qstatus = vfio_ap_irq_enable(q, status & 0x07, vcpu);
-- 
2.53.0

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

[sashiko-bot]

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

[PATCH v5 2/4] s390/vfio-ap: Fix failure to release IRQ notification eventfd contexts

[Anthony Krowiak]

When userspace registers IRQ notification eventfds via the
VFIO_DEVICE_SET_IRQS ioctl, vfio_ap_set_request_irq() and
vfio_ap_set_cfg_change_irq() each call eventfd_ctx_fdget(), which
takes a reference on the eventfd_ctx and stores it in
matrix_mdev->req_trigger and matrix_mdev->cfg_chg_trigger
respectively.

These references are dropped only when userspace explicitly replaces
or clears them via a subsequent SET_IRQS call.  If the device is
closed without that explicit teardown - because the guest exits,
the VM process crashes, or the device file is simply closed -
neither vfio_ap_mdev_close_device() nor the remove path releases
these references.  The eventfd_ctx backing objects and their
associated file references therefore leak for the lifetime of the
kernel.

Fix this by introducing vfio_ap_mdev_release_eventfds() and calling
it from vfio_ap_mdev_close_device() after vfio_ap_mdev_unset_kvm().
The VFIO core guarantees that close_device is called before
vfio_unregister_group_dev() returns in the remove path, so fixing
close_device is sufficient to cover both teardown paths.

Note:
~~~~
The matrix_dev->mdevs lock must be held during the call to
vfio_ap_mdev_release_eventfds(). There is a small window between the calls
to vfio_ap_mdev_unset_kvm() which gets and releases the update locks
and the acquisition of the matrix_dev->mdevs_lock mutex during which
it is possible - although highly unlikely during normal operation - whereby
a concurrent SET_IRQS call can get in.

Taking matrix_dev->mdevs_lock around vfio_ap_mdev_release_eventfds()
is sufficient to make this race-free. The SET_IRQS ioctl path writes
req_trigger and cfg_chg_trigger only from vfio_ap_mdev_ioctl(), which
holds mdevs_lock for its entire duration and always calls
eventfd_ctx_put() on the previous value before storing the new one.

Any number of concurrent SET_IRQS calls during the window between
vfio_ap_mdev_unset_kvm() and the acquisition of mdevs_lock are
therefore safe: each ioctl invocation puts the reference it found and
installs a new one, leaving exactly one live reference in the field
when it releases the lock. When release_eventfds subsequently acquires
mdevs_lock it finds that single surviving reference and puts it.
Conversely, a SET_IRQS call that loses the race and blocks on
mdevs_lock will find the field NULL after release_eventfds finishes,
take ownership of the reference it just created, and install it into a
field that will never be read again - a transient leak. To close that
final case, callers must ensure no new SET_IRQS ioctls can be issued
after close_device() is called, which the VFIO core guarantees by
releasing the device file before invoking close_device().

Fixes: bf48961f6f48e ("s390/vfio-ap: realize the VFIO_DEVICE_SET_IRQS ioctl")
Cc: stable@vger.kernel.org
Signed-off-by: Anthony Krowiak <akrowiak@linux.ibm.com>
Reviewed-by: Matthew Rosato <mjrosato@linux.ibm.com>
---
 drivers/s390/crypto/vfio_ap_ops.c | 16 ++++++++++++++++
 1 file changed, 16 insertions(+)

diff --git a/drivers/s390/crypto/vfio_ap_ops.c b/drivers/s390/crypto/vfio_ap_ops.c
index 8fb0476e3d39..a4980d993b68 100644
--- a/drivers/s390/crypto/vfio_ap_ops.c
+++ b/drivers/s390/crypto/vfio_ap_ops.c
@@ -2167,12 +2167,28 @@ static int vfio_ap_mdev_open_device(struct vfio_device *vdev)
 	return vfio_ap_mdev_set_kvm(matrix_mdev, vdev->kvm);
 }
 
+static void vfio_ap_mdev_release_eventfds(struct ap_matrix_mdev *matrix_mdev)
+{
+	if (matrix_mdev->req_trigger) {
+		eventfd_ctx_put(matrix_mdev->req_trigger);
+		matrix_mdev->req_trigger = NULL;
+	}
+	if (matrix_mdev->cfg_chg_trigger) {
+		eventfd_ctx_put(matrix_mdev->cfg_chg_trigger);
+		matrix_mdev->cfg_chg_trigger = NULL;
+	}
+}
+
 static void vfio_ap_mdev_close_device(struct vfio_device *vdev)
 {
 	struct ap_matrix_mdev *matrix_mdev =
 		container_of(vdev, struct ap_matrix_mdev, vdev);
 
 	vfio_ap_mdev_unset_kvm(matrix_mdev);
+
+	mutex_lock(&matrix_dev->mdevs_lock);
+	vfio_ap_mdev_release_eventfds(matrix_mdev);
+	mutex_unlock(&matrix_dev->mdevs_lock);
 }
 
 static void vfio_ap_mdev_request(struct vfio_device *vdev, unsigned int count)
-- 
2.53.0

Re: [PATCH v5 2/4] s390/vfio-ap: Fix failure to release IRQ notification eventfd contexts

[sashiko-bot]

> When userspace registers IRQ notification eventfds via the
> VFIO_DEVICE_SET_IRQS ioctl, vfio_ap_set_request_irq() and
> vfio_ap_set_cfg_change_irq() each call eventfd_ctx_fdget(), which
> takes a reference on the eventfd_ctx and stores it in
> matrix_mdev->req_trigger and matrix_mdev->cfg_chg_trigger
> respectively.
> 
> These references are dropped only when userspace explicitly replaces
> or clears them via a subsequent SET_IRQS call.  If the device is
> closed without that explicit teardown - because the guest exits,
> the VM process crashes, or the device file is simply closed -
> neither vfio_ap_mdev_close_device() nor the remove path releases
> these references.  The eventfd_ctx backing objects and their
> associated file references therefore leak for the lifetime of the
> kernel.
> [ ... ]
> releasing the device file before invoking close_device().
> 
> Fixes: bf48961f6f48e ("s390/vfio-ap: realize the VFIO_DEVICE_SET_IRQS ioctl")
> Cc: stable@vger.kernel.org
> Signed-off-by: Anthony Krowiak <akrowiak@linux.ibm.com>

Sashiko has reviewed this patch and found no issues. It looks great!

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

[PATCH v5 3/4] s390/vfio-ap: Fix unbounded loop in apq_reset_check()

[Anthony Krowiak]

The apq_reset_check() worker polls ap_tapq() in a while(true) loop
waiting for a queue reset to complete. When ap_tapq() returns
AP_RESPONSE_BUSY or AP_RESPONSE_RESET_IN_PROGRESS,
apq_status_check() returns -EBUSY and the loop continues after
sleeping AP_RESET_MAX_WAIT (20ms). There is no upper bound on how
many times the loop iterates, so if the hardware continuously
returns a busy response the worker runs indefinitely.

This is particularly harmful because several callers of
vfio_ap_reset_queue() - such as vfio_ap_mdev_reset_queues(),
vfio_ap_mdev_reset_qlist() and vfio_ap_mdev_remove_queue - call
flush_work() on the queue's reset_work while holding one or more
of the global matrix_dev locks (guests_lock, mdevs_lock) or the
KVM lock. An indefinitely spinning worker permanently blocks access
to all ap_matrix_mdev objects which could hang other guests that are
using them.

Fix this by introducing AP_RESET_MAX_WAIT (2000ms) and breaking out
of the poll loop when elapsed time reaches that threshold.

If the apq_reset_check() did not verify completion of the reset,
the AQIC resources associated with this queue cannot be freed
because the NIB is the active DMA target for AP interrupt
delivery until the reset completes; freeing the pinned page while the
hardware may still write to it would result in a use-after-free
kernel crash. If the reset eventually completes, interrupts will be
terminated, but the pinned NIB page and ISC registration will be leaked.
This is preferable to either a use-after-free kernel crash or waiting
indefinitely and blocking access to all mdevs, hanging the guests to
which they are attached.

There is another bug in this code that is fixed via this patch. A
response code AP_RESPONSE_NORMAL (0) does not indicate that the
queue was zeroized; it only indicates the PQAP-ZAPQ was accepted.
The zeroizing of the queue is done asynchronously. To verify
completion, the following bits in the status word returned from
PQAP-ZAPQ must be verified:

status->irq_enabled == 0
status->queue_empty == 1
status->replies_waiting == 0
status->async == 0

Note that on timeout, q->reset_status will hold the status from the most
recent reset operation so that callers inspecting
q->reset_status.response_code after flush_work() will see the value
and can return an appropriate return code.

Fixes: dd174833e44e ("s390/vfio-ap: remove upper limit on wait for queue reset to complete")
Cc: stable@vger.kernel.org
Signed-off-by: Anthony Krowiak <akrowiak@linux.ibm.com>
---
 drivers/s390/crypto/vfio_ap_ops.c | 97 ++++++++++++++++++++++++++++++-
 1 file changed, 94 insertions(+), 3 deletions(-)

diff --git a/drivers/s390/crypto/vfio_ap_ops.c b/drivers/s390/crypto/vfio_ap_ops.c
index a4980d993b68..6c315d7a0a08 100644
--- a/drivers/s390/crypto/vfio_ap_ops.c
+++ b/drivers/s390/crypto/vfio_ap_ops.c
@@ -34,6 +34,7 @@
 #define AP_IRQ_ENABLED	1
 
 #define AP_RESET_INTERVAL		20	/* Reset sleep interval (20ms)		*/
+#define AP_RESET_MAX_WAIT		2000	/* Maximum wait for reset (2000ms)	*/
 
 static int vfio_ap_mdev_reset_queues(struct ap_matrix_mdev *matrix_mdev);
 static int vfio_ap_mdev_reset_qlist(struct list_head *qlist);
@@ -2025,12 +2026,31 @@ static int apq_status_check(int apqn, struct ap_queue_status *status)
 {
 	switch (status->response_code) {
 	case AP_RESPONSE_NORMAL:
+		/*
+		 * This response code only indicates that the PQAP-ZAPQ has
+		 * been initiated. The following bit settings in the status
+		 * returned from ZAPQ must be verified to indicate that the
+		 * queue has been zeroized.
+		 */
+		if (status->queue_empty && !status->replies_waiting &&
+		    !status->irq_enabled && !status->async)
+			return 0;
+
+		/* Still transitioning; keep waiting */
+		return -EBUSY;
+
 	case AP_RESPONSE_DECONFIGURED:
 	case AP_RESPONSE_CHECKSTOPPED:
+		/*
+		 * If the queue is non-operational, interrupts are not possible
+		 * and AQIC resources can be safely freed.
+		 */
 		return 0;
+
 	case AP_RESPONSE_RESET_IN_PROGRESS:
 	case AP_RESPONSE_BUSY:
 		return -EBUSY;
+
 	case AP_RESPONSE_ASSOC_SECRET_NOT_UNIQUE:
 	case AP_RESPONSE_ASSOC_FAILED:
 		/*
@@ -2041,6 +2061,7 @@ static int apq_status_check(int apqn, struct ap_queue_status *status)
 		 * a value indicating a reset needs to be performed again.
 		 */
 		return -EAGAIN;
+
 	default:
 		WARN(true,
 		     "failed to verify reset of queue %02x.%04x: TAPQ rc=%u\n",
@@ -2050,6 +2071,33 @@ static int apq_status_check(int apqn, struct ap_queue_status *status)
 	}
 }
 
+static void report_aqic_resource_leak(struct vfio_ap_queue *q)
+{
+	if (q->saved_isc != VFIO_AP_ISC_INVALID || q->saved_iova) {
+		if (q->matrix_mdev) {
+			dev_warn_ratelimited(mdev_dev(q->matrix_mdev->mdev),
+					     "Reset timed out for APQN %02x.%04x: leaking AQIC resources (NIB page & GISC) to prevent host crash\n",
+					     AP_QID_CARD(q->apqn),
+					     AP_QID_QUEUE(q->apqn));
+		} else {
+			pr_warn_ratelimited("Reset timed out for APQN %02x.%04x: leaking AQIC resources (NIB page & GISC) to prevent host crash\n",
+					    AP_QID_CARD(q->apqn),
+					    AP_QID_QUEUE(q->apqn));
+		}
+	} else {
+		if (q->matrix_mdev) {
+			dev_warn_ratelimited(mdev_dev(q->matrix_mdev->mdev),
+					     "Reset timed out for APQN %02x.%04x\n",
+					     AP_QID_CARD(q->apqn),
+					     AP_QID_QUEUE(q->apqn));
+		} else {
+			pr_warn_ratelimited("Reset timed out for APQN %02x.%04x\n",
+					    AP_QID_CARD(q->apqn),
+					    AP_QID_QUEUE(q->apqn));
+		}
+	}
+}
+
 #define WAIT_MSG "Waited %dms for reset of queue %02x.%04x (%u, %u, %u)"
 
 static void apq_reset_check(struct work_struct *reset_work)
@@ -2067,6 +2115,47 @@ static void apq_reset_check(struct work_struct *reset_work)
 		ret = apq_status_check(q->apqn, &status);
 		if (ret == -EIO)
 			return;
+		if (elapsed >= AP_RESET_MAX_WAIT) {
+			/*
+			 * If the status check determined that the reset completed
+			 * successfully or the queue is not operational, clean up
+			 * the AQIC resources because queue reset disables
+			 * interrupts and interrupts are not possible on a
+			 * non-operational queue.
+			 */
+			if (!ret)
+				goto done;
+			/*
+			 * Timed out without being able to verify reset completed.
+			 *
+			 * The AQIC resources associated with this queue - the pinned page
+			 * containing the NIB and the registered guest ISC - cannot be freed
+			 * here. The NIB is the active DMA target for AP interrupt delivery
+			 * until the reset completes; freeing the pinned page while the
+			 * hardware may still write to it would result in a use-after-free
+			 * kernel crash.
+			 *
+			 * If the reset eventually completes, interrupts will be terminated
+			 * and the pinned NIB page and ISC registration will be leaked. This
+			 * is preferable to either a use-after-free or waiting indefinitely:
+			 * the caller of apq_reset_check() holds mdevs_lock while flush_work()
+			 * blocks holds the matrix_dev->mdevs_lock mutex, which
+			 * serializes access to all mdev objects system-wide, so blocking
+			 * here would stall all other guests using AP queues.
+			 */
+			report_aqic_resource_leak(q);
+			/*
+			 * Report the actual non-zero hardware response code, or synthesize
+			 * AP_RESPONSE_RESET_IN_PROGRESS if TAPQ completed normally but
+			 * the status bits failed to transition to their post-reset states.
+			 */
+			if (status.response_code == AP_RESPONSE_NORMAL)
+				q->reset_status.response_code = AP_RESPONSE_RESET_IN_PROGRESS;
+			else
+				q->reset_status.response_code = status.response_code;
+
+			return;
+		}
 		if (ret == -EBUSY) {
 			pr_notice_ratelimited(WAIT_MSG, elapsed,
 					      AP_QID_CARD(q->apqn),
@@ -2083,11 +2172,13 @@ static void apq_reset_check(struct work_struct *reset_work)
 				memcpy(&q->reset_status, &status, sizeof(status));
 				continue;
 			}
-			if (q->saved_isc != VFIO_AP_ISC_INVALID)
-				vfio_ap_free_aqic_resources(q);
-			break;
+			goto done;
 		}
 	}
+
+done:
+	if (q->saved_isc != VFIO_AP_ISC_INVALID)
+		vfio_ap_free_aqic_resources(q);
 }
 
 static void vfio_ap_mdev_reset_queue(struct vfio_ap_queue *q)
-- 
2.53.0

Re: [PATCH v5 3/4] s390/vfio-ap: Fix unbounded loop in apq_reset_check()

[sashiko-bot]

Thank you for your contribution! Sashiko AI review found 3 potential issue(s) to consider:
- [High] The loop in apq_reset_check() may exit successfully on timeout without issuing a ZAPQ reset if the queue becomes idle on the final iteration, leaving cryptographic keys intact.
- [High] The mitigation to prevent a DMA Use-After-Free by intentionally leaking the pinned NIB page fails when the VFIO container is closed, leading to host memory corruption.
- [High] When queue reset times out, the `saved_iova` and `saved_isc` fields in `vfio_ap_queue` are not cleared, leading to incorrect unpinning of a future guest's memory when the queue is reassigned.
--

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

    s390/vfio-ap: Fix unbounded loop in apq_reset_check()
    
    This patch introduces a 2000ms timeout to apq_reset_check() to prevent an
    unbounded loop when hardware continuously returns busy responses. It
    intentionally leaks AQIC resources on timeout to avoid a use-after-free,
    and correctly checks status bits for completion of queue zeroization.

> diff --git a/drivers/s390/crypto/vfio_ap_ops.c b/drivers/s390/crypto/vfio_ap_ops.c
> index a4980d993b681..6c315d7a0a088 100644
> --- a/drivers/s390/crypto/vfio_ap_ops.c
> +++ b/drivers/s390/crypto/vfio_ap_ops.c
[ ... ]
> @@ -2067,6 +2115,47 @@ static void apq_reset_check(struct work_struct *reset_work)
>  		ret = apq_status_check(q->apqn, &status);
>  		if (ret == -EIO)
>  			return;
> +		if (elapsed >= AP_RESET_MAX_WAIT) {
> +			/*
> +			 * If the status check determined that the reset completed
> +			 * successfully or the queue is not operational, clean up
> +			 * the AQIC resources because queue reset disables
> +			 * interrupts and interrupts are not possible on a
> +			 * non-operational queue.
> +			 */
> +			if (!ret)
> +				goto done;

[Severity: High]
Does this early exit skip issuing a required ZAPQ retry if the queue happens
to become idle on the final iteration?

If the initial ap_zapq() fails with AP_RESPONSE_BUSY and the queue becomes
idle exactly on the 100th iteration (2000ms), apq_status_check() will return 0.
The timeout check intercepts this with goto done, which successfully exits
without ever executing the else block that re-issues the ap_zapq() command.

Can this leave cryptographic keys intact on a supposedly reset queue, leaking
them to the next assigned guest?

> +			/*
> +			 * Timed out without being able to verify reset completed.
> +			 *
> +			 * The AQIC resources associated with this queue - the pinned page
> +			 * containing the NIB and the registered guest ISC - cannot be freed
> +			 * here. The NIB is the active DMA target for AP interrupt delivery
> +			 * until the reset completes; freeing the pinned page while the
> +			 * hardware may still write to it would result in a use-after-free
> +			 * kernel crash.
> +			 *
> +			 * If the reset eventually completes, interrupts will be terminated
> +			 * and the pinned NIB page and ISC registration will be leaked. This
> +			 * is preferable to either a use-after-free or waiting indefinitely:
> +			 * the caller of apq_reset_check() holds mdevs_lock while flush_work()
> +			 * blocks holds the matrix_dev->mdevs_lock mutex, which
> +			 * serializes access to all mdev objects system-wide, so blocking
> +			 * here would stall all other guests using AP queues.
> +			 */
> +			report_aqic_resource_leak(q);
> +			/*
> +			 * Report the actual non-zero hardware response code, or synthesize
> +			 * AP_RESPONSE_RESET_IN_PROGRESS if TAPQ completed normally but
> +			 * the status bits failed to transition to their post-reset states.
> +			 */
> +			if (status.response_code == AP_RESPONSE_NORMAL)
> +				q->reset_status.response_code = AP_RESPONSE_RESET_IN_PROGRESS;
> +			else
> +				q->reset_status.response_code = status.response_code;
> +
> +			return;

[Severity: High]
Will intentionally leaking the pinned NIB page actually prevent a
use-after-free on host KVM shutdown?

The timeout logic skips vfio_unpin_pages() and returns here to intentionally
leak the page. However, when the KVM guest is shut down and the VFIO container
is closed, the VFIO IOMMU Type1 driver automatically unpins and frees all
remaining pinned pages.

Since the mdev driver does not take an independent reference (e.g., using
get_page()), the physical page is returned to the host buddy allocator while
the AP hardware may still be performing DMA writes to it. Could this still
lead to host memory corruption?

[Severity: High]
Could returning here without clearing saved_iova and saved_isc corrupt a
future guest's memory when the queue is reassigned?

Because the vfio_ap_queue struct outlives the guest, the saved_iova and
saved_isc fields remain populated with the old guest's state. When this queue
is assigned to a new mdev and the new guest calls vfio_ap_irq_enable(),
vfio_ap_free_aqic_resources() will be called against the new q->matrix_mdev.

Will this erroneously unpin the old guest's IOVA from the new guest's VFIO
container, potentially freeing active memory belonging to the new guest and
unregistering the old ISC from the new KVM instance?

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

[PATCH v5 4/4] s390/vfio-ap: Use AP_DOMAINS for adm_add bitmap size in vfio_ap_mdev_cfg_add()

[Anthony Krowiak]

Domain and control domain bitmaps are sized by the AP_DOMAINS constant, not
AP_DEVICES. The two constants are both 256 today so there is no functional
impact, but using the wrong constant is inconsistent with every operation
on aqm/adm bitmaps.

Use AP_DOMAINS to keep the code consistent and correct in case
the two constants ever diverge.

Note:
This patch was submitted in response to a sashiko review comment pointing
out there are other functions besides vfio_ap_mdev_cfg_add(), so there are
fixes included here for those also. The subject line was kept the same
since this is in v2 of this patch.

Signed-off-by: Anthony Krowiak <akrowiak@linux.ibm.com>
---
 drivers/s390/crypto/vfio_ap_ops.c | 8 ++++----
 1 file changed, 4 insertions(+), 4 deletions(-)

diff --git a/drivers/s390/crypto/vfio_ap_ops.c b/drivers/s390/crypto/vfio_ap_ops.c
index 6c315d7a0a08..70c179e9cb19 100644
--- a/drivers/s390/crypto/vfio_ap_ops.c
+++ b/drivers/s390/crypto/vfio_ap_ops.c
@@ -1517,7 +1517,7 @@ static void vfio_ap_mdev_hot_unplug_domain(struct ap_matrix_mdev *matrix_mdev,
 {
 	DECLARE_BITMAP(apqis, AP_DOMAINS);
 
-	bitmap_zero(apqis, AP_DEVICES);
+	bitmap_zero(apqis, AP_DOMAINS);
 	set_bit_inv(apqi, apqis);
 	vfio_ap_mdev_hot_unplug_domains(matrix_mdev, apqis);
 }
@@ -2910,11 +2910,11 @@ static void vfio_ap_mdev_on_cfg_remove(struct ap_config_info *cur_config_info,
 	do_remove |= bitmap_andnot(aqrem,
 				   (unsigned long *)prev_config_info->aqm,
 				   (unsigned long *)cur_config_info->aqm,
-				   AP_DEVICES);
+				   AP_DOMAINS);
 	do_remove |= bitmap_andnot(cdrem,
 				   (unsigned long *)prev_config_info->adm,
 				   (unsigned long *)cur_config_info->adm,
-				   AP_DEVICES);
+				   AP_DOMAINS);
 
 	if (do_remove)
 		vfio_ap_mdev_cfg_remove(aprem, aqrem, cdrem);
@@ -3025,7 +3025,7 @@ static void vfio_ap_mdev_cfg_add(unsigned long *apm_add, unsigned long *aqm_add,
 		bitmap_and(matrix_mdev->aqm_add,
 			   matrix_mdev->matrix.aqm, aqm_add, AP_DOMAINS);
 		bitmap_and(matrix_mdev->adm_add,
-			   matrix_mdev->matrix.adm, adm_add, AP_DEVICES);
+			   matrix_mdev->matrix.adm, adm_add, AP_DOMAINS);
 
 		mutex_unlock(&matrix_dev->mdevs_lock);
 	}
-- 
2.53.0

Re: [PATCH v5 4/4] s390/vfio-ap: Use AP_DOMAINS for adm_add bitmap size in vfio_ap_mdev_cfg_add()

[sashiko-bot]

> Domain and control domain bitmaps are sized by the AP_DOMAINS constant, not
> AP_DEVICES. The two constants are both 256 today so there is no functional
> impact, but using the wrong constant is inconsistent with every operation
> on aqm/adm bitmaps.
> 
> Use AP_DOMAINS to keep the code consistent and correct in case
> the two constants ever diverge.
> 
> Note:
> This patch was submitted in response to a sashiko review comment pointing
> out there are other functions besides vfio_ap_mdev_cfg_add(), so there are
> fixes included here for those also. The subject line was kept the same
> since this is in v2 of this patch.
> 
> Signed-off-by: Anthony Krowiak <akrowiak@linux.ibm.com>

Sashiko has reviewed this patch and found no issues. It looks great!

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

Re: [PATCH v5 4/4] s390/vfio-ap: Use AP_DOMAINS for adm_add bitmap size in vfio_ap_mdev_cfg_add()

[Matthew Rosato]

On 8/31/26 1:14 PM, Anthony Krowiak wrote:
> Domain and control domain bitmaps are sized by the AP_DOMAINS constant, not
> AP_DEVICES. The two constants are both 256 today so there is no functional
> impact, but using the wrong constant is inconsistent with every operation
> on aqm/adm bitmaps.
> 
> Use AP_DOMAINS to keep the code consistent and correct in case
> the two constants ever diverge.
> 
> Note:
> This patch was submitted in response to a sashiko review comment pointing
> out there are other functions besides vfio_ap_mdev_cfg_add(), so there are
> fixes included here for those also. The subject line was kept the same
> since this is in v2 of this patch.
> 
> Signed-off-by: Anthony Krowiak <akrowiak@linux.ibm.com>

Reviewed-by: Matthew Rosato <mjrosato@linux.ibm.com>