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

[PATCH v6 0/5] 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.

Patch 1/4: Fix leak of pinned NIB and registered NISC in 
           vfio_ap_irq_enable/disable()
* Added return codes to vfio_ap_wait_for_irqclear()
  ~ Returns 0 for response codes AP_RESPONSE_NORMAL and 
    AP_RESPONSE_RESET_IN_PROGRESS if the irq_enabled bit is set to 0
    confirming the queue is disabled for interrupts.
  ~ Returns -ENODEV for response code Q_NOT_AVAIL, DECONFIGURED and
    CHECKSTOPPED to indicate the queue is not accessible
  ~ Returns -ETIMEDOUT if the number of retries is exceeded.
  
* Changes to vfio_ap_irq_enable():
  ~ Removed call to vfio_ap_wait_for_irqstate() because the responsibility
    to wait belongs with the guest that initiated the AQIC.
  ~ All AQIC response codes other than AP_RESPONSE_NORMAL indicate the AQIC
    enable call was rejected by the hardware; so, the AQIC
    resources passed as parameters to the AQIC will be freed; i.e., the
    page containing the NIB will be unpinned and the GISC unregistered.
    
* Changes to vfio_ap_irq_disable():
  ~ The AP_RESPONSE_NORMAL switch case added check for return code from 
    vfio_ap_wait_for_irqclear() rather than just freeing AQIC resources. 
    They are now freed only if the return code is 0 or -ENODEV.
     

Patch 3/4: Fix unbounded loop in apq_reset_check()
* Added a done: goto label at the end of apq_reset_check() that frees AQIC
  resources and can be used in all places that free them.
* Added the following return codes to apq_status_check():
  ~ Return -ENODEV for response codes AP_RESPONSE_DECONFIGURED and
    AP_RESPONSE_CHECKSTOPPED. It indicates that the queue is not
    operational; however also tells us zeroization can not be confirmed.
  ~ Return -EAGAIN for response code AP_RESPONSE_BUSY because it is
    unrelated to reset processing and indicates the ZAPQ should be
    executed again. 
* Made changes to the AP_RESET_MAX_WAIT block:
  ~ Now goto done if the return code from apq_status_check() is 0
    or -ENODEV which indicates the queue is not operational, so it is safe
    to free AQIC resources
  ~ Calls a new function, report_aqic_resource_leak() that logs an 
    appropriate error message if reset timed out.
  ~ Sets q->reset_status.response_code to indicate the reset could not
    complete in the case where the response code returned from ZAPQ is
    AP_RESPONSE_NORMAL but the status bits confirming the reset completed
    could not be verified.
* In the -EBUSY code block, replaced the freeing of the AQIC resources
  with goto done;
    
  
Added a patch 5, s390/vfio-ap: fix queue state leakage to guest and host.
* Patch 4/5 opens a hole fixing the unbounded loop in apq_reset_check
  created by commit dd174833e44e ("s390/vfio-ap: remove upper limit on wait
  for queue reset to complete"). That commit was created "ensure both the
  security requirements and prevent resource leakage and corruption in the
  hypervisor. This patch limits the side effects of an unbounded loop, yet
  accomplishes the goals of that commit.

Anthony Krowiak (5):
  s390/vfio-ap: Fix leak of pinned NIB and registered GISC 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()
  s390/vfio-ap: fix queue state leakage to guest and host

 drivers/s390/crypto/vfio_ap_ops.c     | 373 +++++++++++++++++++++-----
 drivers/s390/crypto/vfio_ap_private.h |   3 +
 2 files changed, 305 insertions(+), 71 deletions(-)

-- 
2.53.0

[PATCH v6 1/5] s390/vfio-ap: Fix leak of pinned NIB and registered GISC in vfio_ap_irq_enable/disable()

[Anthony Krowiak]

[-- Warning: decoded text below may be mangled, UTF-8 assumed --]
[-- Attachment #1: Type: text/plain; charset=y, Size: 19285 bytes --]

The PQAP-AQIC instruction is used to enable or disable interrupts for an
AP queue device. When executed on a guest, the SIE intercepts the
instruction and calls a function designated to handle it; in this case,
it's handle_pqap() in the vfio_ap device driver (vfio_ap_ops.c).

A struct kvm_vcpu instance is passed to handle_pqap(); it contains the
registers containing the input parameters to the PQAP-AQIC instruction
executed on the guest:
* GR0 contains the APQN identifying the queue
* GR1 contains the interruption request (IR) bit specifying whether
  interrupts are to be enabled (1) or disabled (0), as well as the
  interruption subclass (ISC).
* GR2 contains the address of the notification indicator byte (NIB)

When the PQAP-AQIC to enable interrupts is intercepted, a page containing
the NIB has to be pinned and the guest ISC (GISC) registered with KVM.
These values are stored with the struct vfio_ap_queue instance in the
saved_iova and saved_isc fields respectively.

There are two functions that get called by handle_pqap():
* vfio_irq_enable() handles enabling IRQs on behalf of a guest
* vfio_irq_disable() handles disabling IRQs on behalf of a guest

Both of these functions have issues related to managing the
saved_iova and saved_isc fields of the vfio_ap_queue object when interrupts
are enabled/disabled for a queue device. If the NIB page is unpinned while
interrupts are still enabled, the page can be pinned to another process.
If the queue signals an interrupt, a DMA wild write is performed to the now
freed page which will result in memory corruption or possibly a kernel
crash. This patch ensures proper handling of the stored NIB page and GISC.

vfio_ap_irq_enable()
~~~~~~~~~~~~~~~~~~~~
The responsibilities of this function are:
1. To set up the input parameters to the PQAP-AQIC instruction to enable
   queue interrupts and ensure they are signaled to the guest.
2. Execute the AQIC instruction on behalf of the guest
3. Return the status word returned from the AQIC instruction to the guest
   along with the appropriate condition code.

The AQIC instruction returns synchronously, so it is the responsibility of
the guest to verify that the queue was enabled for interrupts by using the
PQAP-TAPQ instruction to verify the I-bit (7) in the status word returned
from TAPQ is set to 1 indicating the queue is enabled for interrupts.

After executing the AQIC, vfio_ap_irq_enable() examines the response code
returned and takes action based upon its value:

* AP_RESPONSE_NORMAL:
  ~ The NIB page saved_iova stored by a previous PQAP-AQIC enable is
    unpinned and the saved_isc is unregistered. This is correct behavior
    because the response code indicates the enable request was accepted,
    .
    This response code is returned synchronously, but it is up to the
    guest to ensure the queue is enabled by executing PQAP-TAPQ and
    verifying the I-bit (7) in the status returned from TAPQ is set to
    one, which indicates the queue is enabled for interrupts.
  ~ Store the address of the page containing the NIB and the
    GISC passed to PQAP-AQIC in the saved_iova and saved_isc fields of
    the vfio_ap_queue instance representing the queue.

* All other PQAP-AQIC response codes:
  ~ These response codes indicate that the hardware rejected the PQAP-AQIC
    command, so the input AQIC resources for the enable attempt must be
    freed; i.e., page containing the NIB unpinned and the GISC
    unregistered.
  ~ We don't free the saved_iova nor unregister the saved_isc for the
    following reasons:
    - AP_RESPONSE_OTHERWISE_CHANGED indicates that the queue is already
      enabled or is in the process of being enabled. Presumably this is
      because the response code resulted from enable request made using
      the page address saved in saved_iova and the GISC saved in saved_isc.
      Freeing them could result in a DMA wild write to the now free page.
    - We leave it up to the guest to handle the other response codes. If
      they attempt to enable interrupts again an it succeeds, the saved_iov
      an saved_isc will be freed. In any case, a queue reset zeroize when
      the queue is unbound from the device driver, unassigned from the
      mdev, or the mdev is removed. A successful reset will disable queue
      interrupts. If the reset fails, the resources will be leaked, but
      if that is the case, there's probably something broken causing the
      AP instructions directed at the queue to fail.

vfio_ap_irq_disable()
~~~~~~~~~~~~~~~~~~~~~
The AQIC call returns synchronously to let the caller know whether the
command was accepted by the hardware or not; however, interrupts are
disabled asynchronously. Until the hardware sets the irq_enabled bit in
the AP queue status word to 0, interrupts are not yet disabled despite
the response code from AQIC.

ap_irq_disable() calls vfio_ap_wait_for_irqclear() which executes
PQAP-TAPQ in a loop until it verifies that the irq_enabled bit is 0
or a response code is returned that indicates the queue is not accessible.
It retries the TAPQ up to 5 iterations with a 20ms sleep in between.
If the irq_enabled bit is not set to 0 in that time period, the function
returns.

The problem here is there is no way to know whether the function
verified the irq_enabled bit is 0, returned because the response code
from TAPQ indicated the queue is not accessible, or timed out. In any case,
vfio_ap_irq_disable() unpins the NIB page and unregisters the GISC used to
enable queue interrupts upon return from vfio_wait_for_irqclear().
If it returned due to a time out, hardware may still write to the NIB and
unpinning the page would allow it to be reallocated to a new owner. A
subsequent hardware DMA write to that physical address would corrupt the
new owner's memory - a wild DMA write that could crash or compromise the
host kernel.

The fix is for the vfio_ap_wait to respond with a return code indicating
why it returned and responding accordingly:

0          - confirmed the irq_enabled bit is 0
-ENODEV    - the response code from TAPQ indicates the queue is not
             accessible
-ETIMEDOUT - the function timed out

If the return code is 0 or -ENODEV, the NIB page and GISC previously used
to enable interrupts will be freed; they are no longer needed since
interrupts are either disabled or because interrupts can not be processed
on an inaccessible queue.

If the return code is -ETIMEDOUT, the AQIC resources previously used to
enable interrupts will be leaked to avoid a DMA wild write. This is
perferable to a corrupted kernel or 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 | 191 ++++++++++++++++++++++--------
 1 file changed, 140 insertions(+), 51 deletions(-)

diff --git a/drivers/s390/crypto/vfio_ap_ops.c b/drivers/s390/crypto/vfio_ap_ops.c
index 940c0ff668be..383ec9f5c810 100644
--- a/drivers/s390/crypto/vfio_ap_ops.c
+++ b/drivers/s390/crypto/vfio_ap_ops.c
@@ -31,6 +31,7 @@
 #define AP_QUEUE_IN_USE "in use"
 
 #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);
@@ -226,16 +227,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
- *
- * 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.
+ * vfio_ap_wait_for_irqclear - wait for the IR bit to clear after a disable
+ *
+ * @apqn: the APQN of the queue
+ *
+ * Repeatedly polls the AP queue status via PQAP(TAPQ) every 20ms until the IR
+ * bit is clear, the queue becomes non-operational, or 5 retries are exhausted.
+ *
+ * Because PQAP(AQIC) disable initiates an asynchronous process, a
+ * condition-code 0 completion does not guarantee the IR bit has been cleared.
+ * The host must confirm IR=0 before unpinning the NIB page to avoid a wild
+ * DMA write to a freed page.
+ *
+ * Return:
+ * - 0		if the IR bit is clear (i.e., interrupts are disabled).
+ *
+ * - -ENODEV	if the PQAP-TAPQ response code indicates the queue is not available,
+ *		is deconfigured, or is checkstopped (i.e., not operational).
+ *
+ * - -ETIMEDOUT	the function timed out before the IR bit was cleared.
  */
-static void vfio_ap_wait_for_irqclear(int apqn)
+static int vfio_ap_wait_for_irqclear(int apqn)
 {
 	struct ap_queue_status status;
 	int retry = 5;
@@ -246,7 +258,7 @@ static void vfio_ap_wait_for_irqclear(int apqn)
 		case AP_RESPONSE_NORMAL:
 		case AP_RESPONSE_RESET_IN_PROGRESS:
 			if (!status.irq_enabled)
-				return;
+				return 0;
 			fallthrough;
 		case AP_RESPONSE_BUSY:
 			msleep(20);
@@ -257,12 +269,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 -ENODEV;
 		}
 	} 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 disabled for %02x.%04x\n",
+		  __func__, status.response_code,
+		  AP_QID_CARD(apqn), AP_QID_QUEUE(apqn));
+
+	return -ETIMEDOUT;
 }
 
 /**
@@ -289,20 +304,28 @@ static void vfio_ap_free_aqic_resources(struct vfio_ap_queue *q)
 }
 
 /**
- * vfio_ap_irq_disable - disables and clears an ap_queue interrupt
- * @q: The vfio_ap_queue
+ * vfio_ap_irq_disable - disable interrupts for an AP queue
+ * @q: the vfio_ap_queue
  *
- * Uses ap_aqic to disable the interruption and in case of success, reset
- * in progress or IRQ disable command already proceeded: calls
- * vfio_ap_wait_for_irqclear() to check for the IRQ bit to be clear
- * and calls vfio_ap_free_aqic_resources() to free the resources associated
- * with the AP interrupt handling.
+ * Issues PQAP(AQIC) to disable interrupts for the AP queue. On success
+ * (AP_RESPONSE_NORMAL or AP_RESPONSE_OTHERWISE_CHANGED), polls via
+ * vfio_ap_wait_for_irqclear() until the IR bit is confirmed clear before
+ * freeing the pinned NIB page and unregistering the guest ISC. This wait is
+ * necessary because AQIC disable is asynchronous: freeing the NIB before IR=0
+ * is confirmed risks a wild DMA write to a freed host page.
  *
- * In the case the AP is busy, or a reset is in progress,
- * retries after 20ms, up to 5 times.
+ * Retries up to 5 times (with 20ms sleep) if the queue is busy or a reset is
+ * in progress.
  *
- * Returns if ap_aqic function failed with invalid, deconfigured or
- * checkstopped AP.
+ * If the IR bit cannot be confirmed clear (timeout), the NIB page and guest
+ * ISC are intentionally leaked. If the page were unpinned and returned to the
+ * allocator, a subsequent hardware DMA write to that physical address would
+ * corrupt memory belonging to a new owner — a wild DMA write that could crash
+ * or compromise the host kernel.
+ *
+ * If the queue is non-operational (deconfigured, checkstopped, not available),
+ * resources are freed immediately since the hardware can no longer write to
+ * the NIB.
  *
  * Return: &struct ap_queue_status
  */
@@ -310,15 +333,38 @@ static struct ap_queue_status vfio_ap_irq_disable(struct vfio_ap_queue *q)
 {
 	union ap_qirq_ctrl aqic_gisa = { .value = 0 };
 	struct ap_queue_status status;
-	int retries = 5;
+	int retries = 5, ret;
 
 	do {
 		status = ap_aqic(q->apqn, aqic_gisa, 0);
 		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.
+			 */
+			ret = vfio_ap_wait_for_irqclear(q->apqn);
+			if (ret == 0 || ret == -ENODEV)
+				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 +372,47 @@ 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 the AQIC instruction failed to disable
+	 * interrupts, or because IR=0 could not be confirmed. In either case
+	 * the NIB page and guest ISC cannot be freed: hardware may still write
+	 * to the NIB, and unpinning the page would allow it to be reallocated
+	 * to a new owner. A subsequent hardware DMA write to that physical
+	 * address would corrupt the new owner's memory — a wild DMA write that
+	 * could crash or compromise the host kernel. The resources are
+	 * therefore intentionally leaked.
+	 */
+	return status;
+
 end_free:
+	/*
+	 * This label is reached because the queue was successfully disabled,
+	 * or because the queue is not operational or not available, 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;
 }
@@ -401,22 +476,29 @@ static int ensure_nib_shared(unsigned long addr)
 }
 
 /**
- * vfio_ap_irq_enable - Enable Interruption for a APQN
+ * vfio_ap_irq_enable - enable interrupts for an AP queue on behalf of a guest
  *
- * @q:	 the vfio_ap_queue holding AQIC parameters
+ * @q:	 the vfio_ap_queue for which interrupts are to be enabled
  * @isc: the guest ISC to register with the GIB interface
- * @vcpu: the vcpu object containing the registers specifying the parameters
- *	  passed to the PQAP(AQIC) instruction.
+ * @vcpu: the vcpu whose registers contain the PQAP(AQIC) parameters
  *
- * Pin the NIB saved in *q
- * Register the guest ISC to GIB interface and retrieve the
- * host ISC to issue the host side PQAP/AQIC
+ * Pins the guest NIB page, registers the guest ISC with the GIB to obtain a
+ * host ISC, and reissues PQAP(AQIC) with the translated host-absolute NIB
+ * address and host ISC on behalf of the guest.
  *
- * status.response_code may be set to AP_RESPONSE_INVALID_ADDRESS in case the
- * vfio_pin_pages or kvm_s390_gisc_register failed.
+ * The condition code and AP-queue status word returned by PQAP(AQIC) are
+ * reflected back to the guest as-is. IRQ state verification (polling until
+ * IR=1) is the responsibility of the guest AP bus, not the host.
  *
- * Otherwise return the ap_queue_status returned by the ap_aqic(),
- * all retry handling will be done by the guest.
+ * Resource management is based solely on whether hardware accepted the new NIB:
+ * - AP_RESPONSE_NORMAL (CC=0): hardware accepted the new NIB; the old pinned
+ *   NIB page and registered guest ISC are freed and the new ones saved.
+ * - All other responses: hardware did not accept the new NIB; the newly pinned
+ *   page and registered ISC are freed and the previously saved resources are
+ *   left intact.
+ *
+ * AP_RESPONSE_INVALID_ADDRESS is returned if vfio_pin_pages() or
+ * kvm_s390_gisc_register() fails before the AQIC instruction is issued.
  *
  * Return: &struct ap_queue_status
  */
@@ -428,11 +510,10 @@ static struct ap_queue_status vfio_ap_irq_enable(struct vfio_ap_queue *q,
 	struct ap_queue_status status = {};
 	struct kvm_s390_gisa *gisa;
 	struct page *h_page;
-	int nisc;
+	int nisc, ret;
 	struct kvm *kvm;
 	phys_addr_t h_nib;
 	dma_addr_t nib;
-	int ret;
 
 	/* Verify that the notification indicator byte address is valid */
 	if (vfio_ap_validate_nib(vcpu, &nib)) {
@@ -489,24 +570,33 @@ 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 */
+		/*
+		 * Hardware accepted the new NIB address (CC=0). The old NIB and
+		 * guest ISC are no longer used by hardware and can be freed.
+		 * The new resources are saved for tracking and future teardown.
+		 *
+		 * IRQ state verification (polling until IR=1) is the
+		 * responsibility of the guest AP bus, not the host. The
+		 * condition code and status word are reflected back to the
+		 * guest to respond to the PQAP-AQIC instruction.
+		 */
 		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 */
+	default:
+		/*
+		 * Hardware did not accept the new NIB (CC=3 or error). The
+		 * previously saved NIB and guest ISC remain active and must
+		 * not be freed. Release the newly pinned page and registered
+		 * ISC that were prepared for this (rejected) request.
+		 */
 		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;
-	default:
-		pr_warn("%s: apqn %04x: response: %02x\n", __func__, q->apqn,
-			status.response_code);
-		vfio_ap_irq_disable(q);
-		break;
 	}
 
 	if (status.response_code != AP_RESPONSE_NORMAL) {
@@ -635,7 +725,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 v6 1/5] s390/vfio-ap: Fix leak of pinned NIB and registered GISC 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] Guest-reachable WARN_ONCE macros violate KVM guidelines and can cause a host denial of service (DoS) under panic_on_warn.

Pre-existing issues:
- [High] Pinned NIB page and registered GISC are permanently leaked when a queue is removed from the host AP configuration or becomes physically unavailable.
--

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

This commit updates the interrupt enable and disable handlers for vfio_ap
to correctly manage pinned NIB pages and GISC registrations. It introduces 
an asynchronous wait process for interrupt clearing to prevent wild DMA 
writes, leaving resources allocated if the hardware doesn't confirm the 
operation.

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

[ ... ]

> @@ -257,12 +269,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 -ENODEV;
>  		}
>  	} 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 disabled for %02x.%04x\n",
> +		  __func__, status.response_code,
> +		  AP_QID_CARD(apqn), AP_QID_QUEUE(apqn));
> +
> +	return -ETIMEDOUT;
>  }

[Severity: High]
Does this allow an unprivileged guest to trigger a host panic?

According to KVM subsystem guidelines, guest-reachable WARN_ON or WARN_ONCE
macros are prohibited because they can crash the host kernel on systems
configured with panic_on_warn.

If a guest forces a delay by issuing a ZAPQ reset (which can take up to
2000ms), and immediately follows it with a PQAP-AQIC disable instruction,
this 100ms TAPQ polling loop (5 retries * 20ms) will time out. 

Could this be converted to pr_warn_once() to avoid host denial of service?

[ ... ]

> @@ -326,18 +372,47 @@ 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--);

[Severity: High]
Does this share the same risk of triggering a host panic as the warning above?

Similar to the timeout case, if the AP returns statuses like DECONFIGURED
or CHECKSTOPPED, this guest-reachable path triggers a WARN_ONCE. Should
these be converted to pr_warn_once() to prevent a potential host DoS?

[ ... ]

[Severity: High]
This isn't a bug introduced by this patch, but does the vfio_ap driver leak
the pinned NIB page and registered GISC when a queue is physically removed
or becomes unavailable?

Looking at vfio_ap_mdev_remove_queue(), if a queue with active interrupts is
removed from the host AP configuration, the bit test fails and we skip
calling vfio_ap_mdev_reset_queue(q):

	/*
	 * If the queue is not in the host's AP configuration, then resetting
	 * it will fail with response code 01, (APQN not valid); so, let's make
	 * sure it is in the host's config.
	 */
	if (test_bit_inv(apid, (unsigned long *)matrix_dev->info.apm) &&
	    test_bit_inv(apqi, (unsigned long *)matrix_dev->info.aqm)) {
		vfio_ap_mdev_reset_queue(q);
		flush_work(&q->reset_work);
	}

done:
	if (matrix_mdev)
		vfio_ap_unlink_queue_fr_mdev(q);

	dev_set_drvdata(&apdev->device, NULL);
	kfree(q);

Because the reset is bypassed, vfio_ap_free_aqic_resources(q) is never
called, permanently leaking the page and KVM routing entries.

Similarly, in vfio_ap_mdev_reset_queue():

	default:
		WARN(true,
		     "PQAP/ZAPQ for %02x.%04x failed with invalid rc=%u\n",
		     AP_QID_CARD(q->apqn), AP_QID_QUEUE(q->apqn),
		     status.response_code);
	}

If ap_zapq() returns AP_RESPONSE_Q_NOT_AVAIL (0x01) because the queue is 
physically unavailable, the default case is hit. It logs a warning but fails 
to call vfio_ap_free_aqic_resources(q), leaking the resources.

Could this lead to memory exhaustion and depletion of KVM ISCs over time if
devices are dynamically reconfigured?

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

Re: [PATCH v6 1/5] s390/vfio-ap: Fix leak of pinned NIB and registered GISC in vfio_ap_irq_enable/disable()

[Anthony Krowiak]

On 9/4/26 5:51 AM, sashiko-bot@kernel.org wrote:
> Thank you for your contribution! Sashiko AI review found 2 potential issue(s) to consider:
>
> New issues:
> - [High] Guest-reachable WARN_ONCE macros violate KVM guidelines and can cause a host denial of service (DoS) under panic_on_warn.
>
> Pre-existing issues:
> - [High] Pinned NIB page and registered GISC are permanently leaked when a queue is removed from the host AP configuration or becomes physically unavailable.
> --
>
> commit ea9124e6c06c371c42de8146e0e3493413dc0829
> Author: Anthony Krowiak <akrowiak@linux.ibm.com>
> s390/vfio-ap: Fix leak of pinned NIB and registered GISC in vfio_ap_irq_enable/disable()
>
> This commit updates the interrupt enable and disable handlers for vfio_ap
> to correctly manage pinned NIB pages and GISC registrations. It introduces
> an asynchronous wait process for interrupt clearing to prevent wild DMA
> writes, leaving resources allocated if the hardware doesn't confirm the
> operation.
>
>> diff --git a/drivers/s390/crypto/vfio_ap_ops.c b/drivers/s390/crypto/vfio_ap_ops.c
>> index 940c0ff668bee..383ec9f5c8107 100644
>> --- a/drivers/s390/crypto/vfio_ap_ops.c
>> +++ b/drivers/s390/crypto/vfio_ap_ops.c
> [ ... ]
>
>> @@ -257,12 +269,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 -ENODEV;
>>   		}
>>   	} 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 disabled for %02x.%04x\n",
>> +		  __func__, status.response_code,
>> +		  AP_QID_CARD(apqn), AP_QID_QUEUE(apqn));
>> +
>> +	return -ETIMEDOUT;
>>   }
> [Severity: High]
> Does this allow an unprivileged guest to trigger a host panic?
>
> According to KVM subsystem guidelines, guest-reachable WARN_ON or WARN_ONCE
> macros are prohibited because they can crash the host kernel on systems
> configured with panic_on_warn.
>
> If a guest forces a delay by issuing a ZAPQ reset (which can take up to
> 2000ms), and immediately follows it with a PQAP-AQIC disable instruction,
> this 100ms TAPQ polling loop (5 retries * 20ms) will time out.
>
> Could this be converted to pr_warn_once() to avoid host denial of service?

This problem has been reported and responded to for all six versions of
this patch as well as others in the series. I discovered while doing some
testing/tracing of manually injected error conditions and discovered that
this is a real possibility; so all of the code locations that are 
reachable from
a guest that log WARN messages need to be fixed by replacing them with
pr_warn_rate_limited() or dev_warn_rate_limited calls.

>
> [ ... ]
>
>> @@ -326,18 +372,47 @@ 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--);
> [Severity: High]
> Does this share the same risk of triggering a host panic as the warning above?
>
> Similar to the timeout case, if the AP returns statuses like DECONFIGURED
> or CHECKSTOPPED, this guest-reachable path triggers a WARN_ONCE. Should
> these be converted to pr_warn_once() to prevent a potential host DoS?

See above

>
> [ ... ]
>
> [Severity: High]
> This isn't a bug introduced by this patch, but does the vfio_ap driver leak
> the pinned NIB page and registered GISC when a queue is physically removed
> or becomes unavailable?
>
> Looking at vfio_ap_mdev_remove_queue(), if a queue with active interrupts is
> removed from the host AP configuration, the bit test fails and we skip
> calling vfio_ap_mdev_reset_queue(q):
>
> 	/*
> 	 * If the queue is not in the host's AP configuration, then resetting
> 	 * it will fail with response code 01, (APQN not valid); so, let's make
> 	 * sure it is in the host's config.
> 	 */
> 	if (test_bit_inv(apid, (unsigned long *)matrix_dev->info.apm) &&
> 	    test_bit_inv(apqi, (unsigned long *)matrix_dev->info.aqm)) {
> 		vfio_ap_mdev_reset_queue(q);
> 		flush_work(&q->reset_work);
> 	}
>
> done:
> 	if (matrix_mdev)
> 		vfio_ap_unlink_queue_fr_mdev(q);
>
> 	dev_set_drvdata(&apdev->device, NULL);
> 	kfree(q);
>
> Because the reset is bypassed, vfio_ap_free_aqic_resources(q) is never
> called, permanently leaking the page and KVM routing entries.
>
> Similarly, in vfio_ap_mdev_reset_queue():
>
> 	default:
> 		WARN(true,
> 		     "PQAP/ZAPQ for %02x.%04x failed with invalid rc=%u\n",
> 		     AP_QID_CARD(q->apqn), AP_QID_QUEUE(q->apqn),
> 		     status.response_code);
> 	}
>
> If ap_zapq() returns AP_RESPONSE_Q_NOT_AVAIL (0x01) because the queue is
> physically unavailable, the default case is hit. It logs a warning but fails
> to call vfio_ap_free_aqic_resources(q), leaking the resources.
>
> Could this lead to memory exhaustion and depletion of KVM ISCs over time if
> devices are dynamically reconfigured?

This analysis is correct, there is a resource leak in
vfio_ap_mdev_reset_queue() when ap_zapq() returns
AP_RESPONSE_Q_NOT_AVAIL (0x01). That response code falls through
to the default case which only fires a WARN without calling
vfio_ap_free_aqic_resources(q).

Regarding vfio_ap_mdev_remove_queue(): there are two distinct
invocation paths to consider:

Sysfs unbind: the queue is still in the host's AP
configuration, so the test_bit_inv guard passes and
vfio_ap_mdev_reset_queue() is called normally. The ZAPQ will
succeed and resources are freed through the normal reset path.

AP bus scan (queue physically removed): the queue is no
longer in the host's AP configuration, so the test_bit_inv
guard is false and the ZAPQ is intentionally skipped — issuing
it would just return rc=0x01 anyway. Since the hardware is gone
and can no longer DMA-write to the NIB, it is safe to call
vfio_ap_free_aqic_resources(q) directly without a ZAPQ.
This is the fix needed here.

Note that vfio_ap_free_aqic_resources() is safe to call
unconditionally in the AP bus scan path even when q->matrix_mdev
is NULL (i.e., the queue is bound to vfio_ap but not assigned to
any mdev). The WARN_ON guards inside the function will protect
against the unpin and unregister operations. Since interrupts can
only be enabled for a queue assigned to an mdev with an active KVM
guest, saved_iova and saved_isc will not be set in that case,
making the call a safe no-op.

The fix for vfio_ap_mdev_reset_queue() is to add
AP_RESPONSE_Q_NOT_AVAIL alongside the existing
AP_RESPONSE_DECONFIGURED and AP_RESPONSE_CHECKSTOPPED cases
so that resources are freed when the queue is not available.

>

[PATCH v6 2/5] 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 383ec9f5c810..ea0625f10c7e 100644
--- a/drivers/s390/crypto/vfio_ap_ops.c
+++ b/drivers/s390/crypto/vfio_ap_ops.c
@@ -2150,12 +2150,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 v6 2/5] 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/20260904093435.1161402-1-akrowiak@linux.ibm.com?part=2

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

[Anthony Krowiak]

[-- Warning: decoded text below may be mangled, UTF-8 assumed --]
[-- Attachment #1: Type: text/plain; charset=y, Size: 8906 bytes --]

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
would allow it to be reallocated to a new owner. A subsequent
hardware DMA write to that physical address would corrupt the
new owner's memory - a wild DMA write that could crash or compromise the
host kernel.

If the reset eventually completes, interrupts will be
terminated, but the pinned NIB page and ISC registration will be leaked.
This is preferable to a compromised kernel or 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 | 123 ++++++++++++++++++++++++++++--
 1 file changed, 118 insertions(+), 5 deletions(-)

diff --git a/drivers/s390/crypto/vfio_ap_ops.c b/drivers/s390/crypto/vfio_ap_ops.c
index ea0625f10c7e..32b80d91a643 100644
--- a/drivers/s390/crypto/vfio_ap_ops.c
+++ b/drivers/s390/crypto/vfio_ap_ops.c
@@ -2008,12 +2008,47 @@ 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 TAPQ must be verified to confirm that the
+		 * asynchronous portion of the queue zeroization has completed.
+		 */
+		if (status->queue_empty && !status->replies_waiting &&
+		    !status->irq_enabled && !status->async)
+			return 0;
+
+		/* Async zeroization still in progress; keep waiting */
+		return -EBUSY;
+
 	case AP_RESPONSE_DECONFIGURED:
 	case AP_RESPONSE_CHECKSTOPPED:
-		return 0;
+		/*
+		 * The queue is non-operational: interrupts are not possible so
+		 * AQIC resources can be safely freed. However, zeroization
+		 * cannot be confirmed because all status bits are zeroed when
+		 * these response codes are returned — there is no way to
+		 * distinguish a zeroized queue from one that has not been
+		 * zeroized. Return -ENODEV to signal that AQIC resources should
+		 * be freed but that zeroization has not been confirmed.
+		 */
+		return -ENODEV;
+
 	case AP_RESPONSE_RESET_IN_PROGRESS:
-	case AP_RESPONSE_BUSY:
+		/*
+		 * A reset is in progress. It may be the reset we issued or one
+		 * issued prior to ours; either way, once it completes the queue
+		 * will be zeroized, so keep waiting.
+		 */
 		return -EBUSY;
+
+	case AP_RESPONSE_BUSY:
+		/*
+		 * The queue is busy with something unrelated to a reset and our
+		 * ZAPQ was rejected outright. Re-issue the ZAPQ.
+		 */
+		return -EAGAIN;
+
 	case AP_RESPONSE_ASSOC_SECRET_NOT_UNIQUE:
 	case AP_RESPONSE_ASSOC_FAILED:
 		/*
@@ -2024,6 +2059,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",
@@ -2033,6 +2069,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)
@@ -2050,6 +2113,54 @@ 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) {
+			/*
+			 * Zeroization confirmed (ret == 0): the TAPQ status bits
+			 * indicate the async portion of the ZAPQ completed
+			 * successfully. Free AQIC resources and return.
+			 *
+			 * Queue non-operational (ret == -ENODEV): the queue is
+			 * deconfigured or checkstopped; interrupts are not
+			 * possible so AQIC resources can be safely freed.
+			 * Zeroization cannot be confirmed in this state, but the
+			 * queue cannot generate interrupts, so the NIB page is
+			 * no longer a DMA target and it is safe to free it.
+			 */
+			if (!ret || ret == -ENODEV)
+				goto done;
+			/*
+			 * Timed out without being able to verify zapq 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 wild DMA write that could
+			 * corrupt host memory.
+			 *
+			 * 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 wild DMA
+			 * write or waiting indefinitely: flush_work() callers hold
+			 * the matrix_dev->mdevs_lock mutex which serializes access
+			 * to all mdev objects system-wide, so blocking here would
+			 * hang all guests to which those mdevs are attached.
+			 */
+			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),
@@ -2066,11 +2177,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 v6 3/5] s390/vfio-ap: Fix unbounded loop in apq_reset_check()

[sashiko-bot]

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

New issues:
- [High] Dangling AQIC resources after a reset timeout cause arbitrary IOVA unpinning in a subsequent guest's VFIO container.

Pre-existing issues:
- [High] Early return on an unknown hardware response code (-EIO) silently leaks AQIC resources and fails to update the queue's reset status.
--

commit fec86a42ae1833abad22bf816bcbaaa29965a88e
Author: Anthony Krowiak <akrowiak@linux.ibm.com>
s390/vfio-ap: Fix unbounded loop in apq_reset_check()

This commit fixes an unbounded loop in the queue reset worker by introducing a
maximum wait time of 2000ms. It also adds logic to verify the asynchronous
completion of queue zeroization and intentionally leaks AQIC resources on
timeout to avoid a potential host crash from wild DMA writes.

> diff --git a/drivers/s390/crypto/vfio_ap_ops.c b/drivers/s390/crypto/vfio_ap_ops.c
> index ea0625f10c7e8..32b80d91a6431 100644
> --- a/drivers/s390/crypto/vfio_ap_ops.c
> +++ b/drivers/s390/crypto/vfio_ap_ops.c

[ ... ]

> @@ -2050,6 +2113,54 @@ static void apq_reset_check(struct work_struct *reset_work)
>  		ret = apq_status_check(q->apqn, &status);
>  		if (ret == -EIO)
>  			return;

[Severity: High]
This is a pre-existing issue, but does this early return on an unknown
hardware response code (-EIO) silently leak AQIC resources?

By returning immediately, this path bypasses the resource cleanup in
vfio_ap_free_aqic_resources(). This leaves q->saved_iova dangling and fails
to update q->reset_status.response_code. 

Since the status remains 0 (AP_RESPONSE_NORMAL), callers might incorrectly 
believe the queue is securely zeroed. If the queue is then reassigned to a 
new guest, could the next successful reset unpin the old IOVA from the new 
guest's IOMMU domain and corrupt memory?

> +		if (elapsed >= AP_RESET_MAX_WAIT) {
> +			/*
> +			 * Zeroization confirmed (ret == 0): the TAPQ status bits
> +			 * indicate the async portion of the ZAPQ completed
> +			 * successfully. Free AQIC resources and return.
> +			 *
> +			 * Queue non-operational (ret == -ENODEV): the queue is
> +			 * deconfigured or checkstopped; interrupts are not
> +			 * possible so AQIC resources can be safely freed.
> +			 * Zeroization cannot be confirmed in this state, but the
> +			 * queue cannot generate interrupts, so the NIB page is
> +			 * no longer a DMA target and it is safe to free it.
> +			 */
> +			if (!ret || ret == -ENODEV)
> +				goto done;
> +			/*
> +			 * Timed out without being able to verify zapq 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 wild DMA write that could
> +			 * corrupt host memory.
> +			 *
> +			 * 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 wild DMA
> +			 * write or waiting indefinitely: flush_work() callers hold
> +			 * the matrix_dev->mdevs_lock mutex which serializes access
> +			 * to all mdev objects system-wide, so blocking here would
> +			 * hang all guests to which those mdevs are attached.
> +			 */
> +			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]
Does this early return on timeout leave dangling AQIC resources that could
cause arbitrary IOVA unpinning in a subsequent guest's VFIO container?

When the worker exits on timeout without clearing the dangling q->saved_iova
field, and the adapter is unassigned from the current guest and hot-plugged
into a new guest, q->matrix_mdev is updated to the new guest.

When the new guest successfully resets the queue, vfio_ap_free_aqic_resources()
will be called. Because q->matrix_mdev is now the new guest's mdev and
q->saved_iova is still the old guest's IOVA, it seems this would erroneously 
unpin an arbitrary IOVA from the new guest's VFIO container:

vfio_ap_free_aqic_resources() {
    ...
    if (q->saved_iova && !WARN_ON(!q->matrix_mdev)) {
        vfio_unpin_pages(&q->matrix_mdev->vdev, q->saved_iova, 1);
        q->saved_iova = 0;
    }
}

[ ... ]

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

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

[Anthony Krowiak]

On 9/4/26 5:48 AM, sashiko-bot@kernel.org wrote:
> Thank you for your contribution! Sashiko AI review found 2 potential issue(s) to consider:
>
> New issues:
> - [High] Dangling AQIC resources after a reset timeout cause arbitrary IOVA unpinning in a subsequent guest's VFIO container.
>
> Pre-existing issues:
> - [High] Early return on an unknown hardware response code (-EIO) silently leaks AQIC resources and fails to update the queue's reset status.
> --
>
> commit fec86a42ae1833abad22bf816bcbaaa29965a88e
> Author: Anthony Krowiak <akrowiak@linux.ibm.com>
> s390/vfio-ap: Fix unbounded loop in apq_reset_check()
>
> This commit fixes an unbounded loop in the queue reset worker by introducing a
> maximum wait time of 2000ms. It also adds logic to verify the asynchronous
> completion of queue zeroization and intentionally leaks AQIC resources on
> timeout to avoid a potential host crash from wild DMA writes.
>
>> diff --git a/drivers/s390/crypto/vfio_ap_ops.c b/drivers/s390/crypto/vfio_ap_ops.c
>> index ea0625f10c7e8..32b80d91a6431 100644
>> --- a/drivers/s390/crypto/vfio_ap_ops.c
>> +++ b/drivers/s390/crypto/vfio_ap_ops.c
> [ ... ]
>
>> @@ -2050,6 +2113,54 @@ static void apq_reset_check(struct work_struct *reset_work)
>>   		ret = apq_status_check(q->apqn, &status);
>>   		if (ret == -EIO)
>>   			return;
> [Severity: High]
> This is a pre-existing issue, but does this early return on an unknown
> hardware response code (-EIO) silently leak AQIC resources?

This will be fixed in patch 1 of the next (v6) of the patch series.

>
> By returning immediately, this path bypasses the resource cleanup in
> vfio_ap_free_aqic_resources(). This leaves q->saved_iova dangling and fails
> to update q->reset_status.response_code.
>
> Since the status remains 0 (AP_RESPONSE_NORMAL), callers might incorrectly
> believe the queue is securely zeroed. If the queue is then reassigned to a
> new guest, could the next successful reset unpin the old IOVA from the new
> guest's IOMMU domain and corrupt memory?
>
>> +		if (elapsed >= AP_RESET_MAX_WAIT) {
>> +			/*
>> +			 * Zeroization confirmed (ret == 0): the TAPQ status bits
>> +			 * indicate the async portion of the ZAPQ completed
>> +			 * successfully. Free AQIC resources and return.
>> +			 *
>> +			 * Queue non-operational (ret == -ENODEV): the queue is
>> +			 * deconfigured or checkstopped; interrupts are not
>> +			 * possible so AQIC resources can be safely freed.
>> +			 * Zeroization cannot be confirmed in this state, but the
>> +			 * queue cannot generate interrupts, so the NIB page is
>> +			 * no longer a DMA target and it is safe to free it.
>> +			 */
>> +			if (!ret || ret == -ENODEV)
>> +				goto done;
>> +			/*
>> +			 * Timed out without being able to verify zapq 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 wild DMA write that could
>> +			 * corrupt host memory.
>> +			 *
>> +			 * 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 wild DMA
>> +			 * write or waiting indefinitely: flush_work() callers hold
>> +			 * the matrix_dev->mdevs_lock mutex which serializes access
>> +			 * to all mdev objects system-wide, so blocking here would
>> +			 * hang all guests to which those mdevs are attached.
>> +			 */
>> +			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]
> Does this early return on timeout leave dangling AQIC resources that could
> cause arbitrary IOVA unpinning in a subsequent guest's VFIO container?
>
> When the worker exits on timeout without clearing the dangling q->saved_iova
> field, and the adapter is unassigned from the current guest and hot-plugged
> into a new guest, q->matrix_mdev is updated to the new guest.
>
> When the new guest successfully resets the queue, vfio_ap_free_aqic_resources()
> will be called. Because q->matrix_mdev is now the new guest's mdev and
> q->saved_iova is still the old guest's IOVA, it seems this would erroneously
> unpin an arbitrary IOVA from the new guest's VFIO container:

The scenario described cannot occur. When the adapter is
unassigned from the mdev, unassign_adapter_store() calls
vfio_ap_mdev_hot_unplug_adapter() which calls
vfio_ap_mdev_hot_unplug_adapters(). That function calls
vfio_ap_mdev_unlink_adapter(), which removes the queue from
the mdev's hash table, but q->matrix_mdev is not set to NULL
until vfio_ap_unlink_mdev_fr_queue() is called later. Before
that, vfio_ap_mdev_reset_qlist() is called, which issues a
fresh ZAPQ for the queue while q->matrix_mdev is still valid.
If that ZAPQ completes successfully, vfio_ap_free_aqic_resources()
will correctly unpin the old NIB page and unregister the ISC
using the still-valid q->matrix_mdev. So while the reviewer
is correct that the AQIC resources are leaked by the timeout
path, they will be cleaned up by the ZAPQ issued during the
unassign, provided the reset succeeds at that point.

>
> vfio_ap_free_aqic_resources() {
>      ...
>      if (q->saved_iova && !WARN_ON(!q->matrix_mdev)) {
>          vfio_unpin_pages(&q->matrix_mdev->vdev, q->saved_iova, 1);
>          q->saved_iova = 0;
>      }
> }
>
> [ ... ]
>

[PATCH v6 4/5] 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>
Reviewed-by: Jason J. Herne <jjherne@linux.ibm.com>
Reviewed-by: Matthew Rosato <mjrosato@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 32b80d91a643..6a964f82c8e8 100644
--- a/drivers/s390/crypto/vfio_ap_ops.c
+++ b/drivers/s390/crypto/vfio_ap_ops.c
@@ -1499,7 +1499,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);
 }
@@ -2915,11 +2915,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);
@@ -3030,7 +3030,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 v6 4/5] 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/20260904093435.1161402-1-akrowiak@linux.ibm.com?part=4

[PATCH v6 5/5] s390/vfio-ap: fix queue state leakage to guest and host

[Anthony Krowiak]

Commit dd174833e44e ("s390/vfio-ap: remove upper limit on wait for queue
reset to complete") removed the upper bound on the wait for a queue reset
to complete in apq_reset_check(), thus allowing the function to loop
indefinitely. The reason given was to ensure both the security requirements
and prevent resource leakage and corruption in the hypervisor.

That is a legitimate concern; however, functions initiating the reset all
hold the matrix_dev->mdevs_lock which guards access to all of the mdevs
under the control of the vfio_ap device driver. Blocking of access prevents
a system administrator from configuring the mdevs (i.e.,
assigning/unassigning adapters, domains and control domains via the mdev's
sysfs interfaces) and may hang any guest that is started using one of
the mdevs to supply its AP configuration.

This patch addresses two issues:
1. Limits the potential hang condition to a single mdev/guest.

2. Prevents leakage of the internal state of a queue to the host or
   any guest started using an mdev to supply its AP configuration.

The key code changes made to address these issues:

1. _queue_passable() accepted reset status response codes
   AP_RESPONSE_DECONFIGURED and AP_RESPONSE_CHECKSTOPPED as passable
   states in addition to AP_RESPONSE_NORMAL. Neither DECONFIGURED nor
   CHECKSTOPPED confirms that the queue was zeroized; only
   AP_RESPONSE_NORMAL (0) - set by apq_reset_check() after TAPQ status bit
   verification - does. To fix this, the passable condition will be limited
   to AP_RESPONSE_NORMAL only.

2. Added a reset_max_wait field to struct vfio_ap_queue. This value
   specifies the maximum length of time to wait for reset completion. A
   value of zero indicates wait until the reset completes.

3. apq_reset_check() used the compile-time constant AP_RESET_MAX_WAIT
   as a hard timeout, causing the reset polling loop to give up and
   mark the queue non-normal even when the caller required confirmed
   zeroization. Replace the constant with the reset_max_wait field from
   the vfio_ap_queue object. When set to AP_RESET_MAX_WAIT, the behaviour
   is unchanged; when set to 0 (new unbind path) the loop runs until
   zeroization is confirmed regardless of elapsed time.

4. Introducing a reset_max_wait value of zero to mean "wait indefinitely"
   required changes to apq_reset_check() beyond simply replacing the
   AP_RESET_MAX_WAIT constant. When the max wait block is bypassed, the
   function must still exit the loop when apq_status_check() returns 0
   (zeroization confirmed) or -ENODEV (device gone), so an explicit
   goto done is added for those two cases. Additionally, the -EBUSY/-EAGAIN
   re-ZAPQ branch used continue followed by an unconditional goto done,
   which caused the loop to exit immediately after a ZAPQ retry rather
   than continuing to poll; both are removed so polling continues
   correctly when the max wait block is bypassed.

5. vfio_ap_mdev_probe_queue() only zeroed the reset_status field at
   probe time; it did not reset the queue itself, so a queue inherited
   whatever state the firmware left it in. Call vfio_ap_mdev_reset_queue()
   and flush_work() at probe to guarantee a clean queue before it can
   be assigned to a guest. Symmetrically, vfio_ap_mdev_remove_queue()
   freed the queue structure immediately after unlinking it even if the
   queue had not been confirmed zeroized. Delay kfree() until after a
   blocking reset (reset_max_wait = 0) when the queue is not already
   confirmed clean, preventing host exposure to stale queue state.
   This, of course, will block the unbinding of the queue until zeroization
   is confirmed, but that is preferable to blocking access to all of the
   mdevs under the vfio_ap device driver's control.

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     | 39 ++++++++++++++++++---------
 drivers/s390/crypto/vfio_ap_private.h |  3 +++
 2 files changed, 29 insertions(+), 13 deletions(-)

diff --git a/drivers/s390/crypto/vfio_ap_ops.c b/drivers/s390/crypto/vfio_ap_ops.c
index 6a964f82c8e8..e054fd4a9497 100644
--- a/drivers/s390/crypto/vfio_ap_ops.c
+++ b/drivers/s390/crypto/vfio_ap_ops.c
@@ -781,14 +781,14 @@ static bool _queue_passable(struct vfio_ap_queue *q)
 	if (!q)
 		return false;
 
-	switch (q->reset_status.response_code) {
-	case AP_RESPONSE_NORMAL:
-	case AP_RESPONSE_DECONFIGURED:
-	case AP_RESPONSE_CHECKSTOPPED:
-		return true;
-	default:
-		return false;
-	}
+	/*
+	 * A queue is only passable if zeroization was confirmed by
+	 * apq_reset_check() via TAPQ status bit verification. This is
+	 * indicated by reset_status.response_code == AP_RESPONSE_NORMAL (0).
+	 * This is to protect against leaking the internal state of the queue
+	 * to the guest.
+	 */
+	return q->reset_status.response_code == AP_RESPONSE_NORMAL;
 }
 
 /*
@@ -2113,7 +2113,7 @@ 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 (q->reset_max_wait && elapsed >= q->reset_max_wait) {
 			/*
 			 * Zeroization confirmed (ret == 0): the TAPQ status bits
 			 * indicate the async portion of the ZAPQ completed
@@ -2161,6 +2161,8 @@ static void apq_reset_check(struct work_struct *reset_work)
 
 			return;
 		}
+		if (!ret || ret == -ENODEV)
+			goto done;
 		if (ret == -EBUSY) {
 			pr_notice_ratelimited(WAIT_MSG, elapsed,
 					      AP_QID_CARD(q->apqn),
@@ -2175,9 +2177,7 @@ static void apq_reset_check(struct work_struct *reset_work)
 			    ret == -EAGAIN) {
 				status = ap_zapq(q->apqn, 0);
 				memcpy(&q->reset_status, &status, sizeof(status));
-				continue;
 			}
-			goto done;
 		}
 	}
 
@@ -2675,8 +2675,10 @@ int vfio_ap_mdev_probe_queue(struct ap_device *apdev)
 
 	q->apqn = apqn;
 	q->saved_isc = VFIO_AP_ISC_INVALID;
-	memset(&q->reset_status, 0, sizeof(q->reset_status));
+	q->reset_max_wait = AP_RESET_MAX_WAIT;
 	INIT_WORK(&q->reset_work, apq_reset_check);
+	vfio_ap_mdev_reset_queue(q);
+	flush_work(&q->reset_work);
 
 	if (matrix_mdev) {
 		vfio_ap_mdev_link_queue(matrix_mdev, q);
@@ -2759,8 +2761,19 @@ void vfio_ap_mdev_remove_queue(struct ap_device *apdev)
 		vfio_ap_unlink_queue_fr_mdev(q);
 
 	dev_set_drvdata(&apdev->device, NULL);
-	kfree(q);
 	release_update_locks_for_mdev(matrix_mdev);
+
+	if (q->reset_status.response_code != AP_RESPONSE_NORMAL) {
+		/*
+		 * Loop until zeroization of queue is verified so we don't leak
+		 * the internal state of the queue to the caller.
+		 */
+		q->reset_max_wait = 0;
+		vfio_ap_mdev_reset_queue(q);
+		flush_work(&q->reset_work);
+	}
+
+	kfree(q);
 }
 
 /**
diff --git a/drivers/s390/crypto/vfio_ap_private.h b/drivers/s390/crypto/vfio_ap_private.h
index 9bff666b0b35..914ea5d41d33 100644
--- a/drivers/s390/crypto/vfio_ap_private.h
+++ b/drivers/s390/crypto/vfio_ap_private.h
@@ -139,6 +139,8 @@ struct ap_matrix_mdev {
  *		 that need to be reset
  * @reset_status: the status from the last reset of the queue
  * @reset_work: work to wait for queue reset to complete
+ * @reset_max_wait: the max amount of time to wait for a reset to complete;
+ *		    specifying 0 means wait as long as it takes.
  */
 struct vfio_ap_queue {
 	struct ap_matrix_mdev *matrix_mdev;
@@ -150,6 +152,7 @@ struct vfio_ap_queue {
 	struct list_head reset_qnode;
 	struct ap_queue_status reset_status;
 	struct work_struct reset_work;
+	unsigned int reset_max_wait;
 };
 
 int vfio_ap_mdev_register(void);
-- 
2.53.0

Re: [PATCH v6 5/5] s390/vfio-ap: fix queue state leakage to guest and host

[sashiko-bot]

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

New issues:
- [High] `apq_reset_check()` fails to update `q->reset_status` on successful completion, breaking queue assignment and state tracking.
- [High] `matrix_mdev` is accessed after its protecting locks are released in `vfio_ap_mdev_remove_queue()`, leading to a UAF.
- [Medium] Unconditional queue resets during probe and removal bypass the host configuration check, triggering unhandled kernel WARNINGs.

Pre-existing issues:
- [High] Early return on `-EIO` in `apq_reset_check()` bypasses freeing AQIC resources, leading to a resource leak and Use-After-Free.
--

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

s390/vfio-ap: fix queue state leakage to guest and host

This patch modifies the queue reset logic to prevent state leakage to guests
and hosts. It changes the passable state for queues to strictly require
AP_RESPONSE_NORMAL, introduces a variable maximum wait time for resets, and
ensures queues are properly reset and verified during probe and removal.

> diff --git a/drivers/s390/crypto/vfio_ap_ops.c b/drivers/s390/crypto/vfio_ap_ops.c
> index 6a964f82c8e89..e054fd4a94970 100644
> --- a/drivers/s390/crypto/vfio_ap_ops.c
> +++ b/drivers/s390/crypto/vfio_ap_ops.c
> @@ -2113,7 +2113,7 @@ static void apq_reset_check(struct work_struct *reset_work)
>  		ret = apq_status_check(q->apqn, &status);
>  		if (ret == -EIO)
>  			return;

[Severity: High]
This isn't a bug introduced by this patch, but does returning early on -EIO
here bypass resource cleanup at the done: label?

If a hardware failure causes an unrecognized response code,
vfio_ap_free_aqic_resources() is skipped here and in the default switch case of
vfio_ap_mdev_reset_queue(), but q is eventually freed.

Can the guest's gmap notifier remain registered, holding a pointer to q,
resulting in a use-after-free when the notifier dereferences the freed q
pointer?

> -		if (elapsed >= AP_RESET_MAX_WAIT) {
> +		if (q->reset_max_wait && elapsed >= q->reset_max_wait) {
>  			/*
>  			 * Zeroization confirmed (ret == 0): the TAPQ status bits
>  			 * indicate the async portion of the ZAPQ completed

[ ... ]

> @@ -2161,6 +2161,8 @@ static void apq_reset_check(struct work_struct *reset_work)
>  
>  			return;
>  		}
> +		if (!ret || ret == -ENODEV)
> +			goto done;

[Severity: High]
Does this early exit skip updating the queue's reset_status with the actual
final state?

If apq_reset_check() loops until ap_tapq() indicates successful zeroization
(ret == 0), but jumps to done without updating q->reset_status to
AP_RESPONSE_NORMAL, does this mean _queue_passable() will incorrectly reject
successfully zeroized queues?

Furthermore, could this cause the new logic in vfio_ap_mdev_remove_queue() to
evaluate q->reset_status.response_code != AP_RESPONSE_NORMAL to true,
triggering an incorrect redundant reset that blocks indefinitely?

>  		if (ret == -EBUSY) {
>  			pr_notice_ratelimited(WAIT_MSG, elapsed,
>  					      AP_QID_CARD(q->apqn),

[ ... ]

> @@ -2675,8 +2675,10 @@ int vfio_ap_mdev_probe_queue(struct ap_device *apdev)
>  
>  	q->apqn = apqn;
>  	q->saved_isc = VFIO_AP_ISC_INVALID;
> -	memset(&q->reset_status, 0, sizeof(q->reset_status));
> +	q->reset_max_wait = AP_RESET_MAX_WAIT;
>  	INIT_WORK(&q->reset_work, apq_reset_check);
> +	vfio_ap_mdev_reset_queue(q);
> +	flush_work(&q->reset_work);

[Severity: Medium]
Will this unconditional reset during probe trigger a WARN if the probed device
is not in the host's AP configuration?

When an AP device is bound to the vfio_ap driver but is not present in the
host's AP configuration, ap_zapq() fails with response code 0x01.

Does this feed the 0x01 response code into vfio_ap_mdev_reset_queue(), which
falls through to the default switch case and triggers a WARN?

>  
>  	if (matrix_mdev) {
>  		vfio_ap_mdev_link_queue(matrix_mdev, q);

[ ... ]

> @@ -2759,8 +2761,19 @@ void vfio_ap_mdev_remove_queue(struct ap_device *apdev)
>  		vfio_ap_unlink_queue_fr_mdev(q);
>  
>  	dev_set_drvdata(&apdev->device, NULL);
> -	kfree(q);
>  	release_update_locks_for_mdev(matrix_mdev);
> +
> +	if (q->reset_status.response_code != AP_RESPONSE_NORMAL) {

[Severity: High]
Are the locks protecting matrix_mdev dropped before the blocking reset
operations, exposing it to concurrent freeing?

vfio_ap_unlink_queue_fr_mdev() unlinks the queue but leaves the dangling
pointer q->matrix_mdev intact.

If flush_work() blocks indefinitely and a concurrent unbind tears down the
mdev and frees matrix_mdev, will vfio_ap_free_aqic_resources() dereference
q->matrix_mdev->kvm, resulting in a use-after-free?

> +		/*
> +		 * Loop until zeroization of queue is verified so we don't leak
> +		 * the internal state of the queue to the caller.
> +		 */
> +		q->reset_max_wait = 0;
> +		vfio_ap_mdev_reset_queue(q);
> +		flush_work(&q->reset_work);

[Severity: Medium]
Does this fallback reset completely ignore the test_bit_inv() host
configuration check present earlier in this function, hitting the same WARN on
unbind if the device is not in the host's AP configuration?

> +	}
> +
> +	kfree(q);
>  }
>  
>  /**

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

Re: [PATCH v6 5/5] s390/vfio-ap: fix queue state leakage to guest and host

[Anthony Krowiak]

On 9/4/26 5:53 AM, sashiko-bot@kernel.org wrote:
> Thank you for your contribution! Sashiko AI review found 4 potential issue(s) to consider:
>
> New issues:
> - [High] `apq_reset_check()` fails to update `q->reset_status` on successful completion, breaking queue assignment and state tracking.
> - [High] `matrix_mdev` is accessed after its protecting locks are released in `vfio_ap_mdev_remove_queue()`, leading to a UAF.
> - [Medium] Unconditional queue resets during probe and removal bypass the host configuration check, triggering unhandled kernel WARNINGs.
>
> Pre-existing issues:
> - [High] Early return on `-EIO` in `apq_reset_check()` bypasses freeing AQIC resources, leading to a resource leak and Use-After-Free.
> --
>
> commit 0e38aebd52e81cf78709732210a4726b0f1c0dfa
> Author: Anthony Krowiak <akrowiak@linux.ibm.com>
>
> s390/vfio-ap: fix queue state leakage to guest and host
>
> This patch modifies the queue reset logic to prevent state leakage to guests
> and hosts. It changes the passable state for queues to strictly require
> AP_RESPONSE_NORMAL, introduces a variable maximum wait time for resets, and
> ensures queues are properly reset and verified during probe and removal.
>
>> diff --git a/drivers/s390/crypto/vfio_ap_ops.c b/drivers/s390/crypto/vfio_ap_ops.c
>> index 6a964f82c8e89..e054fd4a94970 100644
>> --- a/drivers/s390/crypto/vfio_ap_ops.c
>> +++ b/drivers/s390/crypto/vfio_ap_ops.c
>> @@ -2113,7 +2113,7 @@ static void apq_reset_check(struct work_struct *reset_work)
>>   		ret = apq_status_check(q->apqn, &status);
>>   		if (ret == -EIO)
>>   			return;
> [Severity: High]
> This isn't a bug introduced by this patch, but does returning early on -EIO
> here bypass resource cleanup at the done: label?

The AQIC resources need to be freed here.

>
> If a hardware failure causes an unrecognized response code,
> vfio_ap_free_aqic_resources() is skipped here and in the default switch case of
> vfio_ap_mdev_reset_queue(), but q is eventually freed.
>
> Can the guest's gmap notifier remain registered, holding a pointer to q,
> resulting in a use-after-free when the notifier dereferences the freed q
> pointer?
>
>> -		if (elapsed >= AP_RESET_MAX_WAIT) {
>> +		if (q->reset_max_wait && elapsed >= q->reset_max_wait) {
>>   			/*
>>   			 * Zeroization confirmed (ret == 0): the TAPQ status bits
>>   			 * indicate the async portion of the ZAPQ completed
> [ ... ]
>
>> @@ -2161,6 +2161,8 @@ static void apq_reset_check(struct work_struct *reset_work)
>>   
>>   			return;
>>   		}
>> +		if (!ret || ret == -ENODEV)
>> +			goto done;
> [Severity: High]
> Does this early exit skip updating the queue's reset_status with the actual
> final state?
>
> If apq_reset_check() loops until ap_tapq() indicates successful zeroization
> (ret == 0), but jumps to done without updating q->reset_status to
> AP_RESPONSE_NORMAL, does this mean _queue_passable() will incorrectly reject
> successfully zeroized queues?
>
> Furthermore, could this cause the new logic in vfio_ap_mdev_remove_queue() to
> evaluate q->reset_status.response_code != AP_RESPONSE_NORMAL to true,
> triggering an incorrect redundant reset that blocks indefinitely?

These are valid concerns.

The cases that result in verifying completion of the ZAPQ on a work 
queue are:
AP_RESPONSE_NORMAL
AP_RESPONSE_RESET_IN_PROGRESS
AP_RESPONSE_STATE_CHANGE_IN_PROGRESS

That is what will be in the q->reset_status.response_code. The NORMAL is 
fine,
but the others are not. For ret == 0, the response code needs to be set to
AP_RESPONSE_NORMAL; for ret = -ENODEV, it should be set to whatever
response code was returned from TAPQ in apq_status_check().

>
>>   		if (ret == -EBUSY) {
>>   			pr_notice_ratelimited(WAIT_MSG, elapsed,
>>   					      AP_QID_CARD(q->apqn),
> [ ... ]
>
>> @@ -2675,8 +2675,10 @@ int vfio_ap_mdev_probe_queue(struct ap_device *apdev)
>>   
>>   	q->apqn = apqn;
>>   	q->saved_isc = VFIO_AP_ISC_INVALID;
>> -	memset(&q->reset_status, 0, sizeof(q->reset_status));
>> +	q->reset_max_wait = AP_RESET_MAX_WAIT;
>>   	INIT_WORK(&q->reset_work, apq_reset_check);
>> +	vfio_ap_mdev_reset_queue(q);
>> +	flush_work(&q->reset_work);
> [Severity: Medium]
> Will this unconditional reset during probe trigger a WARN if the probed device
> is not in the host's AP configuration?

The queue is probed by the vfio_ap driver precisely because the AP bus
scan detected it in the host's AP configuration, so ap_zapq() returning
AP_RESPONSE_Q_NOT_AVAIL at probe time is not possible

>
> When an AP device is bound to the vfio_ap driver but is not present in the
> host's AP configuration, ap_zapq() fails with response code 0x01.
>
> Does this feed the 0x01 response code into vfio_ap_mdev_reset_queue(), which
> falls through to the default switch case and triggers a WARN?
>
>>   
>>   	if (matrix_mdev) {
>>   		vfio_ap_mdev_link_queue(matrix_mdev, q);
> [ ... ]
>
>> @@ -2759,8 +2761,19 @@ void vfio_ap_mdev_remove_queue(struct ap_device *apdev)
>>   		vfio_ap_unlink_queue_fr_mdev(q);
>>   
>>   	dev_set_drvdata(&apdev->device, NULL);
>> -	kfree(q);
>>   	release_update_locks_for_mdev(matrix_mdev);
>> +
>> +	if (q->reset_status.response_code != AP_RESPONSE_NORMAL) {
> [Severity: High]
> Are the locks protecting matrix_mdev dropped before the blocking reset
> operations, exposing it to concurrent freeing?
>
> vfio_ap_unlink_queue_fr_mdev() unlinks the queue but leaves the dangling
> pointer q->matrix_mdev intact.
>
> If flush_work() blocks indefinitely and a concurrent unbind tears down the
> mdev and frees matrix_mdev, will vfio_ap_free_aqic_resources() dereference
> q->matrix_mdev->kvm, resulting in a use-after-free?

Yes. This will be removed. There is also concern with hanging the function
indefinitely.

>
>> +		/*
>> +		 * Loop until zeroization of queue is verified so we don't leak
>> +		 * the internal state of the queue to the caller.
>> +		 */
>> +		q->reset_max_wait = 0;
>> +		vfio_ap_mdev_reset_queue(q);
>> +		flush_work(&q->reset_work);
> [Severity: Medium]
> Does this fallback reset completely ignore the test_bit_inv() host
> configuration check present earlier in this function, hitting the same WARN on
> unbind if the device is not in the host's AP configuration?

That will go away too.

>
>> +	}
>> +
>> +	kfree(q);
>>   }
>>   
>>   /**