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

[PATCH v8 0/6] 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 v7 => v8:
~~~~~~~~~~~~~~~~~~~
Patch 1/6: Fix leaks of pinned NIB and registered GISC
* Added return codes to vfio_ap_wait_for_irqclear()
  ~ Returns 0 when the IR bit is clear confirming interrupts
    are disabled
  ~ Returns -ENODEV for response codes AP_RESPONSE_Q_NOT_AVAIL,
    AP_RESPONSE_DECONFIGURED, and AP_RESPONSE_CHECKSTOPPED to
    indicate the queue is not operational
  ~ Returns -ETIMEDOUT if retries are exhausted or TAPQ returns
    AP_RESPONSE_ASSOC_SECRET_NOT_UNIQUE or
    AP_RESPONSE_ASSOC_FAILED indicating the IR bit cannot be
    confirmed clear
  ~ Returns -EIO if TAPQ returns an invalid response code
* Updated vfio_ap_irq_disable() to handle the return code from
  vfio_ap_wait_for_irqclear()
  ~ Frees AQIC resources on return code 0 or -ENODEV
  ~ Intentionally leaks AQIC resources on -ETIMEDOUT or -EIO
    to prevent a wild DMA write that could corrupt host memory
  ~ Returns AP_RESPONSE_OTHERWISE_CHANGED to the guest on
    -ETIMEDOUT if the TAPQ response code is AP_RESPONSE_NORMAL
    or AP_RESPONSE_BUSY, to signal the disable did not complete
* Added handling for previously unhandled AQIC response codes
  ~ AP_RESPONSE_STATE_CHANGE_IN_PROGRESS: retries after 20ms,
    same as RESET_IN_PROGRESS and BUSY
  ~ AP_RESPONSE_INVALID_GISA, AP_RESPONSE_ASSOC_SECRET_NOT_
    UNIQUE, AP_RESPONSE_ASSOC_FAILED: AQIC resources are
    intentionally leaked since the disable was rejected and
    hardware still holds the NIB address
* Updated apq_status_check() to correctly interpret TAPQ status
  ~ AP_RESPONSE_NORMAL: verifies queue_empty, replies_waiting,
    irq_enabled, and async bits before returning 0; returns
    -EBUSY if async zeroization is still in progress
  ~ AP_RESPONSE_DECONFIGURED, AP_RESPONSE_CHECKSTOPPED: returns
    -ENODEV (AQIC resources freed but zeroization unconfirmed)
  ~ AP_RESPONSE_BUSY: returns -EAGAIN to re-issue ZAPQ, no
    longer falls through to ASSOC_SECRET_NOT_UNIQUE case
* Updated apq_reset_check() to handle -ENODEV from
  apq_status_check()
  ~ On -ENODEV, frees AQIC resources and records TAPQ status
    in q->reset_status to mark the queue as non-passable
  ~ On -EIO, intentionally leaks AQIC resources and records
    TAPQ status to mark the queue as non-passable
* Updated vfio_ap_mdev_reset_queue() to handle response codes
  ~ Added AP_RESPONSE_Q_NOT_AVAIL alongside DECONFIGURED and
    CHECKSTOPPED to free AQIC resources for all non-operational
    queue states
  ~ Removed AP_RESPONSE_BUSY (not a valid ZAPQ response code)
* Updated vfio_ap_mdev_remove_queue() to free AQIC resources
  when the queue is not in the host AP configuration via an
  else branch to the test_bit_inv host-config guard
* Updated unmap_iova() to fall back to a bounded queue reset
  (ZAPQ) when vfio_ap_irq_disable() leaks the NIB page, since
  the vfio core requires the page be unpinned before dma_unmap
  returns; after the reset the NIB is unconditionally freed

Patch 3/6: Fix unbounded loop in apq_reset_check()
* apq_reset_check()
  ~ On timeout, q->reset_status.response_code is set to
    AP_RESPONSE_RESET_IN_PROGRESS to signal incomplete reset
    and trigger ZAPQ re-issue on next reset attempt
  ~ AQIC resources (pinned NIB page and GISC registration)
    are intentionally leaked on timeout to prevent a wild
    DMA write that could corrupt or crash the host kernel
  ~ report_aqic_resource_leak() added to emit a ratelimited
    warning when resources are leaked
* Introduced apq_reset_finalize() to update q->reset_status
  with the confirmed TAPQ end-state and free AQIC resources
  ~ Sets response_code to AP_RESPONSE_NORMAL only when
    apq_status_check() returns 0 (zeroization confirmed)
  ~ Leaves non-zero response code intact for -ENODEV so
    _queue_passable() correctly returns false
* Fixed incorrect treatment of AP_RESPONSE_NORMAL from
  PQAP(ZAPQ) as zeroization confirmation; AP_RESPONSE_NORMAL
  only means the ZAPQ was accepted — completion requires
  all four TAPQ status bits to be verified
* Fixed AP_RESPONSE_BUSY handling in apq_status_check()
  ~ Was falling through to ASSOC_SECRET_NOT_UNIQUE; now
    returns -EAGAIN to re-issue the ZAPQ independently
* Elapsed time counter is reset to 0 when ZAPQ is re-issued
  due to -EAGAIN or RESET_IN_PROGRESS or
  STATE_CHANGE_IN_PROGRESS

Patch 5/6: Fix queue state leakage to guest and host
* Restricted _queue_passable() to return true only when
  reset_status.response_code == AP_RESPONSE_NORMAL (0)
  ~ Removed AP_RESPONSE_DECONFIGURED and
    AP_RESPONSE_CHECKSTOPPED as passable states; neither
    confirms zeroization and passing such a queue could
    leak key material from a prior guest or host operation
* Added on_qstate_transition callback to struct ap_driver
  and implemented vfio_ap_on_qstate_transition()
  ~ AP bus invokes the callback during scan when a queue
    transitions between configured/deconfigured or
    checkstopped/not-checkstopped states
  ~ On AP_QUEUE_CONFIG_ON or AP_QUEUE_CHKSTOP_OFF the queue
    is reset and zeroized; if reset succeeds the guest APCB
    is updated to plug the queue into the guest configuration
* Added ap_qstate_transition struct to ap_bus.h carrying the
  queue pointer and new_state enum
* vfio_ap_mdev_probe_queue() now calls
  vfio_ap_mdev_reset_queue() and flush_work() at probe time
  to guarantee a clean queue before assignment to a guest
* Fixed ordering of kfree(q) vs release_update_locks_for_mdev()
  in vfio_ap_mdev_remove_queue() to avoid use-after-free

Patch 6/6: replace guest-reachable WARNs with ratelimited warnings
* Replaced all WARN/WARN_ONCE calls in guest-reachable paths
  with ratelimited warning functions to prevent log flooding
  by a malicious or misbehaving guest
* Introduced five reporting helper functions:
  ~ report_tapq_rc() - invalid/unexpected PQAP(TAPQ) rc;
    used in vfio_ap_wait_for_irqclear() and
    apq_status_check()
  ~ report_irqclear_timeout() - timeout waiting for IR bit
    to clear after PQAP(AQIC) disable
  ~ report_aqic_disable_error() - failed PQAP(AQIC) disable;
    replaces three WARN_ONCE calls for non-operational queue,
    rejected disable, and retry exhaustion cases
  ~ report_zapq_rc() - invalid PQAP(ZAPQ) rc in
    vfio_ap_mdev_reset_queue()
  ~ report_gisc_unregister_failure() - failure to unregister
    guest ISC when matrix_mdev or kvm context is NULL
* Changed signatures of vfio_ap_wait_for_irqclear() and
  apq_status_check() to accept struct vfio_ap_queue * instead
  of apqn so mdev context is available for reporting
* Augmented VFIO_AP_DBF_WARN() sites in vfio_ap_irq_enable()
  and handle_pqap() with companion dev_warn_ratelimited() or
  pr_warn_ratelimited() calls for dmesg visibility 

Anthony Krowiak (6):
  s390/vfio-ap: Fix leaks of pinned NIB and registered GISC
  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
  s390/vfio-ap: replace guest-reachable WARNs with ratelimited warnings

 drivers/s390/crypto/ap_bus.c          |  51 ++
 drivers/s390/crypto/ap_bus.h          |  29 +
 drivers/s390/crypto/vfio_ap_drv.c     |   1 +
 drivers/s390/crypto/vfio_ap_ops.c     | 739 ++++++++++++++++++++++----
 drivers/s390/crypto/vfio_ap_private.h |   2 +
 5 files changed, 704 insertions(+), 118 deletions(-)

-- 
2.53.0

[PATCH v8 1/6] s390/vfio-ap: Fix leaks of pinned NIB and registered GISC

[Anthony Krowiak]

Several code paths in the vfio_ap driver failed to free the AQIC
resources — the pinned guest NIB page and the registered guest ISC
used to enable interrupts for a queue — when a queue became unavailable
or when unexpected response codes were returned. This could cause memory
exhaustion and depletion of KVM interrupt subclass registrations over time
with repeated dynamic AP reconfiguration. On the other hand, there are
situations whereby these resources must be intentionally leaked.
If the page were unpinned and returned to the allocator, a subsequent
wild DMA-write to that physical address would corrupt memory belonging
to the new owner and could crash or compromise the host kernel.

vfio_ap_mdev_reset_queue()
~~~~~~~~~~~~~~~~~~~~~~~~~~
AP_RESPONSE_Q_NOT_AVAIL (0x01) was not handled, causing it to fall
through to the default case which only fired a WARN without calling
vfio_ap_free_aqic_resources(). When ap_zapq() returns this response
code the queue is physically unavailable and can no longer generate
AP interrupts or DMA-write to the NIB. Add AP_RESPONSE_Q_NOT_AVAIL
alongside AP_RESPONSE_DECONFIGURED and AP_RESPONSE_CHECKSTOPPED so
that AQIC resources are freed immediately for all three non-
operational cases.

AP_RESPONSE_BUSY is not a valid response code for a PQAP(ZAPQ)
instruction, so it is removed. This contradicts what is stated
in the following:

commit 411b0109daa52 ("s390/vfio-ap: wait for response code 05 to clear on queue reset")

A careful reading of the valid response code table in the architecture
documentation, however, clearly shows that response code 05 is not valid
for the PQAP-ZAPQ instruction. So in the apq_status_check() - which
examines the response codes that can be returned from PQAP-TAPQ - if the
response code is AP_RESPONSE_BUSY (05), it will return -EAGAIN which
instructs the caller (apq_reset_check()) to re-issue the PQAP-ZAPQ
instruction.

vfio_ap_mdev_remove_queue()
~~~~~~~~~~~~~~~~~~~~~~~~~~~
When the AP bus scan detects a queue is no longer in the host AP
configuration, vfio_ap_mdev_remove_queue() skips the ZAPQ since
issuing it would return AP_RESPONSE_Q_NOT_AVAIL anyway. However,
vfio_ap_free_aqic_resources() was also never called, leaking the
pinned NIB page and registered guest ISC. Since the hardware is
gone and can no longer DMA-write to the NIB, it is safe to call
vfio_ap_free_aqic_resources() directly. Add an else branch to the
host-config test_bit_inv guard to free AQIC resources when the
queue is not in the host AP configuration. If the queue is not
assigned to an mdev, vfio_ap_free_aqic_resources() is a no-op.

apq_reset_check()
~~~~~~~~~~~~~~~~~
Keep in mind that apq_reset_check() - which calls
apq_status_check() - is called on a work queue when the status response
code from the ZAPQ is AP_RESPONSE_NORMAL, AP_RESPONSE_RESET_IN_PROGRESS,
or AP_RESPONSE_STATE_CHANGE_IN_PROGRESS. If the apq_reset_check() times
out before it can be verified that the asynchronous portion of the
reset completed, the response code returned will be one of those above,
so the status from the PQAP(TAPQ) is copied to q->reset_status so the
queue is marked as not-passable and not passed through to a guest.
~~~~~~~~~~~~~~~~~~~~~~~~~~~

vfio_ap_irq_disable()
~~~~~~~~~~~~~~~~~~~~~
The valid response codes for PQAP(AQIC) include
AP_RESPONSE_STATE_CHANGE_IN_PROGRESS (0x0a),
AP_RESPONSE_INVALID_GISA (0x08),
AP_RESPONSE_ASSOC_SECRET_NOT_UNIQUE (0x35), and
AP_RESPONSE_ASSOC_FAILED (0x36), none of which were explicitly
handled. All four fell through to the default case and jumped to
end_free freed the AQIC resources. These each should be handled
differently:

* AP_RESPONSE_STATE_CHANGE_IN_PROGRESS indicates a transient
  condition, analogous to AP_RESPONSE_RESET_IN_PROGRESS and
  AP_RESPONSE_BUSY. It is added to the same case as the
  RESET_IN_PROGRESS AND RESPONSE_BUSY whereby the process
  sleeps for 20ms and the AQIC gets re-executed.

* AP_RESPONSE_INVALID_GISA indicates the AQIC instruction was
  rejected due to an invalid GISA address. The disable did not take
  effect and the hardware still holds the NIB page address, so the
  AQIC resources must be leaked. If the NIB page were unpinned and returned
  to the allocator, a subsequent wild DMA-write to that physical
  address would corrupt memory belonging to the new owner and could crash
  or compromise the host kernel; so the AQIC resources must be
  intentionally leaked.

* AP_RESPONSE_ASSOC_SECRET_NOT_UNIQUE and AP_RESPONSE_ASSOC_FAILED
  are asynchronous response codes from a previously executed
  association instruction. All subsequent AQIC calls will end with the
  asynchronous response code until the queue is reset; therefore the AQIC
  disable was not executed and the hardware still holds the NIB address.
  If the NIB page was unpinned and returned to the allocator, a subsequent
  wild DMA-write to that physical address would corrupt memory belonging
  to the new owner and could crash or compromise the host kernel;
  so the AQIC resources must be intentionally leaked.

After a return from the vfio_ap_wait_for_clear(), vfio_ap_irq_disable()
immediately frees the AQIC resources; however, vfio_ap_wait_for_clear()
can return for a number of reasons which require a different response:

* Timed out waiting for the I-bit (bit 7) in the AP queue status word
  returned from the TAPQ instruction to be cleared (indicates interrupts
  are disabled). In this case, the hardware still holds the NIB address.
  If the NIB page were unpinned and returned to the allocator, a subsequent
  wild DMA-write to that physical address would corrupt memory belonging
  to the new owner and could crash or compromise the host kernel;
  so the AQIC resources must be intentionally leaked.

* The response code from TAPQ is one of AP_RESPONSE_NORMAL,
  AP_RESPONSE_Q_NOT_AVAIL, AP_RESPONSE_DECONFIGURED, OR
  AP_RESPONSE_CHECKSTOPPED. In this case, the response code indicates the
  interrupts are disabled or the queue is either not in the host's
  AP configuration or is not functional. In any case, it is safe to free
  the AQIC resources.

* An invalid response code was returned from TAPQ. In this case, the
  hardware may still hold the NIB address. If so and the NIB page was
  unpinned and returned to the allocator, a subsequent wild DMA-write to
  that physical address would corrupt memory belonging to the new owner and
  could crash or compromise the host kernel; so the AQIC resources must be
  intentionally leaked.

The solution here is to change the vfio_ap_wait_for_clear() to reply with
a return code indicating the result, thus allowing vfio_ap_irq_disable()
to respond accordingly:
 * 0:		interrupt disablement is verified
 * -ENODEV:	the queue is not functional
 * -ETIMEDOUT:	the function timeout without verifying interrupts disabled
 * -EIO:	an invalid response code was returned from TAPQ

unmap_iova()
~~~~~~~~~~~~
Calls vfio_ap_irq_disable() to disable interrupts for the queue, but does
not check the result. If the IRQ disable failed or could not be
confirmed, then vfio_ap_irq_disable() leaks the NIB to prevent a wild
DMA-write; however, the vfio core requires that NIB page to be unpinned
before the dma_unmap returns, or it will BUG_ON after 10 re-notification
rounds.

The fix is to fall back to a bounded queue reset and zeroize (ZAPQ) which
zeroizes the NIB pointer in the hardware, eliminating the DMA risk that
justified the leak. Once the reset finishes or times out with a reset
confirmed in-progress, the hardware no longer holds a reference to
saved_iova and it is safe to unpin unconditionally.

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 | 365 +++++++++++++++++++++++++-----
 1 file changed, 304 insertions(+), 61 deletions(-)

diff --git a/drivers/s390/crypto/vfio_ap_ops.c b/drivers/s390/crypto/vfio_ap_ops.c
index 940c0ff668be..6998bd0c88a2 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,27 +227,48 @@ 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
+ * @tapq_status:	used to return the TAPQ status to the caller
+ *
+ * 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, or TAPQ
+ *		returned AP_RESPONSE_ASSOC_SECRET_NOT_UNIQUE or
+ *		AP_RESPONSE_ASSOC_FAILED, which mean the instruction was not
+ *		executed and will continue to be returned for all subsequent
+ *		instructions except ZAPQ until the queue is reset. Since IR=0
+ *		cannot be confirmed, the NIB must be treated as a potential DMA
+ *		target and leaked rather than freed.
+ *
+ * -EIO		PQAP-TAPQ returned an invalid response code
  */
-static void vfio_ap_wait_for_irqclear(int apqn)
+static int vfio_ap_wait_for_irqclear(int apqn, struct ap_queue_status *tapq_status)
 {
 	struct ap_queue_status status;
 	int retry = 5;
 
 	do {
 		status = ap_tapq(apqn, NULL);
+		memcpy(tapq_status, &status, sizeof(status));
 		switch (status.response_code) {
 		case AP_RESPONSE_NORMAL:
 		case AP_RESPONSE_RESET_IN_PROGRESS:
 			if (!status.irq_enabled)
-				return;
+				return 0;
 			fallthrough;
 		case AP_RESPONSE_BUSY:
 			msleep(20);
@@ -254,15 +276,34 @@ static void vfio_ap_wait_for_irqclear(int apqn)
 		case AP_RESPONSE_Q_NOT_AVAIL:
 		case AP_RESPONSE_DECONFIGURED:
 		case AP_RESPONSE_CHECKSTOPPED:
+			WARN_ONCE(1, "%s: tapq rc %02x: %04x\n", __func__,
+				  status.response_code, apqn);
+			return -ENODEV;
+		case AP_RESPONSE_ASSOC_SECRET_NOT_UNIQUE:
+		case AP_RESPONSE_ASSOC_FAILED:
+			/*
+			 * The TAPQ instruction was not executed. Executing TAPQ
+			 * again will result in the same error until the queue
+			 * is reset. We should't, however, reset the queue in
+			 * this context, so log a warning and return -ETIMEDOUT
+			 * since that would happen anyway if we continued to
+			 * execute the TAPQ.
+			 */
+			WARN_ONCE(1, "%s: tapq rc %02x: %04x\n", __func__,
+				  status.response_code, apqn);
+			return -ETIMEDOUT;
 		default:
 			WARN_ONCE(1, "%s: tapq rc %02x: %04x\n", __func__,
 				  status.response_code, apqn);
-			return;
+			return -EIO;
 		}
 	} 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,55 +330,139 @@ 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
  */
 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;
+	struct ap_queue_status status, tapq_status;
+	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, &tapq_status);
+			if (ret == 0 || ret == -ENODEV)
+				goto end_free;
+
+			if (ret == -EIO) {
+				/*
+				 * An unknown TAPQ response code was returned which
+				 * indicates a bug or some type of hardware I/O issue.
+				 * Since we don't know whether queue interrupts were
+				 * disabled or not, the AQIC resources must be leaked.
+				 */
+				memcpy(&status, &tapq_status, sizeof(status));
+				goto end_fail;
+			}
+
+			/* Timed out waiting to confirm interrupts are disabled */
+			if (tapq_status.response_code == AP_RESPONSE_NORMAL ||
+			    tapq_status.response_code == AP_RESPONSE_BUSY) {
+				/*
+				 * If 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.
+				 * AP_RESPONSE_BUSY is not valid for PQAP-AQIC.
+				 *
+				 * Return OTHERWISE_CHANGED to signal to the guest that
+				 * the disable interrupts operation did not complete.
+				 */
+				memset(&status, 0, sizeof(status));
+				status.response_code = AP_RESPONSE_OTHERWISE_CHANGED;
+			} else {
+				/*
+				 * For all other TAPQ response codes,
+				 * return the TAPQ status directly since those
+				 * codes are also valid for AQIC.
+				 */
+				memcpy(&status, &tapq_status, sizeof(status));
+			}
+			goto end_fail;
 		case AP_RESPONSE_RESET_IN_PROGRESS:
 		case AP_RESPONSE_BUSY:
+		case AP_RESPONSE_STATE_CHANGE_IN_PROGRESS:
 			msleep(20);
 			break;
 		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:
+		case AP_RESPONSE_INVALID_GISA:
+		case AP_RESPONSE_ASSOC_SECRET_NOT_UNIQUE:
+		case AP_RESPONSE_ASSOC_FAILED:
 		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 +526,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 +560,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,27 +620,46 @@ 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 */
+		/*
+		 * Interrupts are already enabled or a prior async request is still
+		 * completing. Either way, the hardware's current NIB is the one
+		 * saved in q->saved_iova/q->saved_isc — not the newly prepared
+		 * resources. Release the newly pinned page and registered ISC;
+		 * leave saved_iova and saved_isc intact.
+		 */
+		fallthrough;
+	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) {
+	if (status.response_code != AP_RESPONSE_NORMAL &&
+	    status.response_code != AP_RESPONSE_OTHERWISE_CHANGED) {
 		VFIO_AP_DBF_WARN("%s: PQAP(AQIC) failed with status=%#02x: "
 				 "zone=%#x, ir=%#x, gisc=%#x, f=%#x,"
 				 "gisa=%#x, isc=%#x, apqn=%#04x\n",
@@ -635,7 +785,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);
@@ -1857,8 +2006,28 @@ static void unmap_iova(struct ap_matrix_mdev *matrix_mdev, u64 iova, u64 length)
 	int loop_cursor;
 
 	hash_for_each(qtable->queues, loop_cursor, q, mdev_qnode) {
-		if (q->saved_iova >= iova && q->saved_iova < iova + length)
+		if (q->saved_iova >= iova && q->saved_iova < iova + length) {
 			vfio_ap_irq_disable(q);
+			/*
+			 * If IRQ disable failed or IR=0 could not be confirmed,
+			 * vfio_ap_irq_disable() intentionally leaks the NIB to
+			 * prevent a wild DMA write. But vfio core requires the
+			 * page to be unpinned before dma_unmap returns, or it
+			 * will BUG_ON after 10 re-notification rounds.
+			 *
+			 * Fall back to a bounded queue reset. The ZAPQ zeroizes
+			 * the NIB pointer in hardware, eliminating the DMA risk
+			 * that justified the leak. Once the worker finishes (or
+			 * times out with a reset confirmed in-progress), the
+			 * hardware no longer holds a reference to saved_iova and
+			 * it is safe to unpin unconditionally.
+			 */
+			if (q->saved_iova) {
+				vfio_ap_mdev_reset_queue(q);
+				flush_work(&q->reset_work);
+				vfio_ap_free_aqic_resources(q);
+			}
+		}
 	}
 }
 
@@ -1919,22 +2088,57 @@ 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:
 	case AP_RESPONSE_ASSOC_SECRET_NOT_UNIQUE:
 	case AP_RESPONSE_ASSOC_FAILED:
 		/*
+		 * AP_RESPONSE_BUSY:
+		 * The queue is busy with something unrelated to a reset and our
+		 * ZAPQ was rejected outright. Re-issue the ZAPQ.
+		 *
+		 * AP_RESPONSE_ASSOC_SECRET_NOT_UNIQUE
+		 * AP_RESPONSE_ASSOC_FAILED:
 		 * These asynchronous response codes indicate a PQAP(AAPQ)
 		 * instruction to associate a secret with the guest failed. All
 		 * subsequent AP instructions will end with the asynchronous
-		 * response code until the AP queue is reset; so, let's return
-		 * a value indicating a reset needs to be performed again.
+		 * response code until the AP queue is reset. Re-issue the ZAPQ.
 		 */
 		return -EAGAIN;
+
 	default:
 		WARN(true,
 		     "failed to verify reset of queue %02x.%04x: TAPQ rc=%u\n",
@@ -1959,8 +2163,25 @@ static void apq_reset_check(struct work_struct *reset_work)
 		elapsed += AP_RESET_INTERVAL;
 		status = ap_tapq(q->apqn, NULL);
 		ret = apq_status_check(q->apqn, &status);
-		if (ret == -EIO)
+		if (ret == -EIO) {
+			/*
+			 * TAPQ returned an invalid response code indicating a
+			 * hardware or firmware bug. Since we cannot determine
+			 * whether the queue can still DMA-write to the NIB, the
+			 * AQIC resources are intentionally leaked lest the NIB
+			 * page is reallocated to a new owner. A subsequent
+			 * wild DMA-write to that physical address would corrupt
+			 * the new owner's memory which could crash or compromise
+			 * the host kernel.
+			 *
+			 * Record the TAPQ status so the queue is marked
+			 * not-passable - _queue_passable() checks
+			 * reset_status.response_code == AP_RESPONSE_NORMAL -
+			 * and the queue is not passed through to a guest.
+			 */
+			memcpy(&q->reset_status, &status, sizeof(status));
 			return;
+		}
 		if (ret == -EBUSY) {
 			pr_notice_ratelimited(WAIT_MSG, elapsed,
 					      AP_QID_CARD(q->apqn),
@@ -1977,8 +2198,12 @@ 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);
+			/*
+			 * We end up here when the ZAPQ has completed. ZAPQ
+			 * disables interrupts, so the AQIC resources must be
+			 * freed; otherwise they will be leaked.
+			 */
+			vfio_ap_free_aqic_resources(q);
 			break;
 		}
 	}
@@ -1995,18 +2220,27 @@ static void vfio_ap_mdev_reset_queue(struct vfio_ap_queue *q)
 	switch (status.response_code) {
 	case AP_RESPONSE_NORMAL:
 	case AP_RESPONSE_RESET_IN_PROGRESS:
-	case AP_RESPONSE_BUSY:
 	case AP_RESPONSE_STATE_CHANGE_IN_PROGRESS:
 		/*
 		 * Let's verify whether the ZAPQ completed successfully on a work queue.
 		 */
 		queue_work(system_long_wq, &q->reset_work);
 		break;
+	case AP_RESPONSE_Q_NOT_AVAIL:
 	case AP_RESPONSE_DECONFIGURED:
 	case AP_RESPONSE_CHECKSTOPPED:
 		vfio_ap_free_aqic_resources(q);
 		break;
 	default:
+		/*
+		 * An invalid response code indicates a hardware or firmware bug.
+		 * Since we cannot determine whether the queue can still
+		 * DMA-write to the NIB, the AQIC resources are intentionally
+		 * leaked lest the NIB page is reallocated to a new owner. A
+		 * subsequent hardware wild DMA-write to that physical address
+		 * would corrupt the new owner's memory which could crash or
+		 * compromise the host kernel.
+		 */
 		WARN(true,
 		     "PQAP/ZAPQ for %02x.%04x failed with invalid rc=%u\n",
 		     AP_QID_CARD(q->apqn), AP_QID_QUEUE(q->apqn),
@@ -2534,6 +2768,15 @@ void vfio_ap_mdev_remove_queue(struct ap_device *apdev)
 	    test_bit_inv(apqi, (unsigned long *)matrix_dev->info.aqm)) {
 		vfio_ap_mdev_reset_queue(q);
 		flush_work(&q->reset_work);
+	} else {
+		/*
+		 * The queue is no longer in the host's AP configuration.
+		 * The hardware cannot DMA-write to the NIB, so it is safe
+		 * to free the AQIC resources directly without issuing a
+		 * ZAPQ. If the queue is not assigned to an mdev,
+		 * vfio_ap_free_aqic_resources() is a no-op.
+		 */
+		vfio_ap_free_aqic_resources(q);
 	}
 
 done:
-- 
2.53.0

[PATCH v8 2/6] 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 6998bd0c88a2..cbe2fb564a7e 100644
--- a/drivers/s390/crypto/vfio_ap_ops.c
+++ b/drivers/s390/crypto/vfio_ap_ops.c
@@ -2295,12 +2295,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

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

[Anthony Krowiak]

s390/vfio-ap: Fix unbounded loop in apq_reset_check()

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, AP_RESPONSE_RESET_IN_PROGRESS, or
AP_RESPONSE_BUSY, apq_status_check() returns -EBUSY and the
loop continues after sleeping AP_RESET_INTERVAL (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_mdev_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 apq_reset_check() times out before verifying completion of
the reset, the AQIC resources associated with the queue cannot
be freed. 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 wild DMA-write to that physical address would corrupt the
new owner's memory and 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.
On timeout, q->reset_status.response_code is set to
AP_RESPONSE_RESET_IN_PROGRESS. This is used internally to
signal that the reset did not complete, and ensures that if
the queue is reset again, the re-issue logic in
apq_reset_check() will re-issue the ZAPQ.

This patch also fixes a bug whereby AP_RESPONSE_NORMAL (0)
returned from PQAP(ZAPQ) was incorrectly treated as
confirmation that the queue was zeroized. AP_RESPONSE_NORMAL
only indicates that the ZAPQ was accepted; zeroization is
performed asynchronously. To confirm completion, the following
bits in the status word returned from PQAP(TAPQ) must all
be verified:

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

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 | 134 ++++++++++++++++++++++++++----
 1 file changed, 118 insertions(+), 16 deletions(-)

diff --git a/drivers/s390/crypto/vfio_ap_ops.c b/drivers/s390/crypto/vfio_ap_ops.c
index cbe2fb564a7e..47d4936fb9d7 100644
--- a/drivers/s390/crypto/vfio_ap_ops.c
+++ b/drivers/s390/crypto/vfio_ap_ops.c
@@ -2123,6 +2123,12 @@ static int apq_status_check(int apqn, struct ap_queue_status *status)
 		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:
 		/*
@@ -2148,8 +2154,59 @@ 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)"
 
+/**
+ * apq_reset_finalize - store final TAPQ status and free AQIC resources.
+ * @q:      the vfio_ap_queue
+ * @status: the final AP queue status returned by PQAP(TAPQ)
+ * @ret:    the return value from apq_status_check()
+ *
+ * Copies the full TAPQ status word to q->reset_status so that all status
+ * bits reflect the confirmed end state of the queue. If ret == 0,
+ * zeroization was confirmed and the response code is overridden with
+ * AP_RESPONSE_NORMAL so that _queue_passable() returns true. For
+ * ret == -ENODEV (DECONFIGURED or CHECKSTOPPED), the non-zero response
+ * code is left intact so _queue_passable() correctly returns false.
+ * AQIC resources are then freed.
+ */
+static void apq_reset_finalize(struct vfio_ap_queue *q,
+			       struct ap_queue_status *status, int ret)
+{
+	memcpy(&q->reset_status, status, sizeof(*status));
+	if (!ret)
+		q->reset_status.response_code = AP_RESPONSE_NORMAL;
+
+	vfio_ap_free_aqic_resources(q);
+}
+
 static void apq_reset_check(struct work_struct *reset_work)
 {
 	int ret = -EBUSY, elapsed = 0;
@@ -2182,6 +2239,58 @@ static void apq_reset_check(struct work_struct *reset_work)
 			memcpy(&q->reset_status, &status, sizeof(status));
 			return;
 		}
+
+		if (!ret || ret == -ENODEV) {
+			/*
+			 * 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.
+			 */
+			apq_reset_finalize(q, &status, ret);
+			return;
+		}
+
+		if (elapsed >= AP_RESET_MAX_WAIT) {
+			/*
+			 * 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);
+			/*
+			 * Zeroization could not be confirmed; set
+			 * reset_status to AP_RESPONSE_RESET_IN_PROGRESS.
+			 * This is used internally to signal that the reset
+			 * did not complete, and ensures that if the queue
+			 * is reset again, the re-issue logic in
+			 * apq_reset_check() will re-issue the ZAPQ.
+			 */
+			q->reset_status.response_code = AP_RESPONSE_RESET_IN_PROGRESS;
+
+			return;
+		}
+
 		if (ret == -EBUSY) {
 			pr_notice_ratelimited(WAIT_MSG, elapsed,
 					      AP_QID_CARD(q->apqn),
@@ -2189,22 +2298,15 @@ static void apq_reset_check(struct work_struct *reset_work)
 					      status.response_code,
 					      status.queue_empty,
 					      status.irq_enabled);
-		} else {
-			if (q->reset_status.response_code == AP_RESPONSE_RESET_IN_PROGRESS ||
-			    q->reset_status.response_code == AP_RESPONSE_BUSY ||
-			    q->reset_status.response_code == AP_RESPONSE_STATE_CHANGE_IN_PROGRESS ||
-			    ret == -EAGAIN) {
-				status = ap_zapq(q->apqn, 0);
-				memcpy(&q->reset_status, &status, sizeof(status));
-				continue;
-			}
-			/*
-			 * We end up here when the ZAPQ has completed. ZAPQ
-			 * disables interrupts, so the AQIC resources must be
-			 * freed; otherwise they will be leaked.
-			 */
-			vfio_ap_free_aqic_resources(q);
-			break;
+			continue;
+		}
+
+		if (ret == -EAGAIN ||
+		    q->reset_status.response_code == AP_RESPONSE_RESET_IN_PROGRESS ||
+		    q->reset_status.response_code == AP_RESPONSE_STATE_CHANGE_IN_PROGRESS) {
+			status = ap_zapq(q->apqn, 0);
+			memcpy(&q->reset_status, &status, sizeof(status));
+			elapsed = 0;
 		}
 	}
 }
-- 
2.53.0

[PATCH v8 4/6] 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 47d4936fb9d7..ffc2d8715bd9 100644
--- a/drivers/s390/crypto/vfio_ap_ops.c
+++ b/drivers/s390/crypto/vfio_ap_ops.c
@@ -1559,7 +1559,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);
 }
@@ -3058,11 +3058,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);
@@ -3173,7 +3173,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

[PATCH v8 5/6] 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 limits the potential hang to the AP_RESET_MAX_WAIT
(2000ms) timeout introduced in the preceding commit, and
prevents leakage of queue state to a guest.

_queue_passable() accepted 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) does. A queue that is not confirmed zeroized must not be
passed through to a guest, as it may contain key material from
a previous guest or host operation.

Since _queue_passable() now rejects queues that are check stopped
or deconfigured, there is no way for a queue bound to the
vfio_ap device driver to pass those through to a guest even
if they are added back to the configuration or the reason they
have check stopped has been fixed. To resolve this issue,
a new on_queue_state_transition callback function is added to
struct ap_driver which is invoked during the AP bus device scan
when a queue device transitions from deconfigured to configured or
check stopped to not check stopped and vice versa. The vfio_ap device
driver provides an implementation that resets and zeroizes the queue when
it transitions to configured or not check stopped and plugs it into the
guest's AP configuration if the reset succeeds.

To fix this, _queue_passable() is limited to returning true
only when reset_status.response_code == AP_RESPONSE_NORMAL.
A new helper, apq_reset_finalize(), is introduced to ensure
q->reset_status correctly reflects the confirmed end state
of the queue. It copies the full TAPQ status word to
q->reset_status and sets q->reset_status.response_code to
AP_RESPONSE_NORMAL only when apq_status_check() returns 0,
confirming zeroization via TAPQ status bit verification.
For -ENODEV (DECONFIGURED or CHECKSTOPPED), the non-zero
response code is left intact, ensuring _queue_passable()
correctly returns false.

Additionally, vfio_ap_mdev_probe_queue() now calls
vfio_ap_mdev_reset_queue() and flush_work() at probe time
to guarantee a clean queue before it can be assigned to
a guest.

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/ap_bus.c          | 51 +++++++++++++++++++++
 drivers/s390/crypto/ap_bus.h          | 29 ++++++++++++
 drivers/s390/crypto/vfio_ap_drv.c     |  1 +
 drivers/s390/crypto/vfio_ap_ops.c     | 64 ++++++++++++++++++++++-----
 drivers/s390/crypto/vfio_ap_private.h |  2 +
 5 files changed, 137 insertions(+), 10 deletions(-)

diff --git a/drivers/s390/crypto/ap_bus.c b/drivers/s390/crypto/ap_bus.c
index d82df5b4e2db..a53cfad3543e 100644
--- a/drivers/s390/crypto/ap_bus.c
+++ b/drivers/s390/crypto/ap_bus.c
@@ -1979,6 +1979,28 @@ static inline void notify_scan_complete(void)
 			 __drv_notify_scan_complete);
 }
 
+/* Helper function for notify_config_changed */
+static int __drv_notify_qstate_transitioned(struct device_driver *drv, void *data)
+{
+	struct ap_driver *ap_drv = to_ap_drv(drv);
+	struct ap_qstate_transition *qstate_trans = data;
+
+	if (try_module_get(drv->owner)) {
+		if (ap_drv->on_qstate_transition)
+			ap_drv->on_qstate_transition(qstate_trans);
+		module_put(drv->owner);
+	}
+
+	return 0;
+}
+
+/* Notify all drivers about a queue state transition */
+static inline void notify_qstate_transitioned(struct ap_qstate_transition *qstate_trans)
+{
+	bus_for_each_drv(&ap_bus_type, NULL, qstate_trans,
+			 __drv_notify_qstate_transitioned);
+}
+
 /*
  * Helper function for ap_scan_bus().
  * Remove card device and associated queue devices.
@@ -1998,6 +2020,7 @@ static inline void ap_scan_rm_card_dev_and_queue_devs(struct ap_card *ac)
  */
 static inline void ap_scan_domains(struct ap_card *ac)
 {
+	struct ap_qstate_transition qstate_trans;
 	struct ap_tapq_hwinfo hwinfo;
 	bool decfg, chkstop;
 	struct ap_queue *aq;
@@ -2093,6 +2116,13 @@ static inline void ap_scan_domains(struct ap_card *ac)
 			spin_unlock_bh(&aq->lock);
 			pr_debug("(%d,%d) queue dev checkstop on\n",
 				 ac->id, dom);
+			/*
+			 * Notify drivers that the queue state has transitioned
+			 * to checkstopped.
+			 */
+			qstate_trans.queue = aq;
+			qstate_trans.new_state = AP_QUEUE_CHKSTOP_ON;
+			notify_qstate_transitioned(&qstate_trans);
 			/* 'receive' pending messages with -EAGAIN */
 			ap_flush_queue(aq);
 			goto put_dev_and_continue;
@@ -2104,6 +2134,13 @@ static inline void ap_scan_domains(struct ap_card *ac)
 			spin_unlock_bh(&aq->lock);
 			pr_debug("(%d,%d) queue dev checkstop off\n",
 				 ac->id, dom);
+			/*
+			 * Notify drivers that the queue state has transitioned
+			 * to not checkstopped.
+			 */
+			qstate_trans.queue = aq;
+			qstate_trans.new_state = AP_QUEUE_CHKSTOP_OFF;
+			notify_qstate_transitioned(&qstate_trans);
 			goto put_dev_and_continue;
 		}
 		/* config state change */
@@ -2117,6 +2154,13 @@ static inline void ap_scan_domains(struct ap_card *ac)
 			spin_unlock_bh(&aq->lock);
 			pr_debug("(%d,%d) queue dev config off\n",
 				 ac->id, dom);
+			/*
+			 * Notify drivers that the queue state has transitioned
+			 * to deconfigured.
+			 */
+			qstate_trans.queue = aq;
+			qstate_trans.new_state = AP_QUEUE_CONFIG_OFF;
+			notify_qstate_transitioned(&qstate_trans);
 			ap_send_config_uevent(&aq->ap_dev, aq->config);
 			/* 'receive' pending messages with -EAGAIN */
 			ap_flush_queue(aq);
@@ -2129,6 +2173,13 @@ static inline void ap_scan_domains(struct ap_card *ac)
 			spin_unlock_bh(&aq->lock);
 			pr_debug("(%d,%d) queue dev config on\n",
 				 ac->id, dom);
+			/*
+			 * Notify drivers that the queue state has transitioned
+			 * to configured.
+			 */
+			qstate_trans.queue = aq;
+			qstate_trans.new_state = AP_QUEUE_CONFIG_ON;
+			notify_qstate_transitioned(&qstate_trans);
 			ap_send_config_uevent(&aq->ap_dev, aq->config);
 			goto put_dev_and_continue;
 		}
diff --git a/drivers/s390/crypto/ap_bus.h b/drivers/s390/crypto/ap_bus.h
index fb4d678336e4..fd2c7be683e3 100644
--- a/drivers/s390/crypto/ap_bus.h
+++ b/drivers/s390/crypto/ap_bus.h
@@ -132,6 +132,29 @@ struct ap_message;
  */
 #define AP_DRIVER_FLAG_DEFAULT 0x0001
 
+/**
+ * ap_queue_state_transition:
+ *
+ * Used to notify a device driver that a queue state transition has occurred.
+ *
+ * @queue:	the queue device whose state transitioned
+ * @new_state:  identifies the new state to which the queue transitioned:
+ *	AP_QUEUE_CONFIG_ON:		from deconfigured to configured
+ *	AP_QUEUE_CONFIG_OFF:		from configured to deconfigured
+ *	AP_QUEUE_CHKSTOPPED_ON:		from not checkstopped to checkstopped
+ *	AP_QUEUE_CHKSTOPPED_OFF:	from checkstopped to not checkstopped
+ */
+struct ap_qstate_transition {
+	struct ap_queue *queue;
+
+	enum {
+		AP_QUEUE_CONFIG_ON,
+		AP_QUEUE_CONFIG_OFF,
+		AP_QUEUE_CHKSTOP_ON,
+		AP_QUEUE_CHKSTOP_OFF,
+	} new_state;
+};
+
 struct ap_driver {
 	struct device_driver driver;
 
@@ -155,6 +178,12 @@ struct ap_driver {
 	void (*on_scan_complete)(struct ap_config_info *new_config_info,
 				 struct ap_config_info *old_config_info);
 
+	/*
+	 * Called during the ap bus scan when a queue state transition is
+	 * detected.
+	 */
+	void (*on_qstate_transition)(struct ap_qstate_transition *qstate_trans);
+
 	struct ap_device_id *ids;
 	unsigned int flags;
 };
diff --git a/drivers/s390/crypto/vfio_ap_drv.c b/drivers/s390/crypto/vfio_ap_drv.c
index 8e69ed286bb9..6a5d97fa9200 100644
--- a/drivers/s390/crypto/vfio_ap_drv.c
+++ b/drivers/s390/crypto/vfio_ap_drv.c
@@ -61,6 +61,7 @@ static struct ap_driver vfio_ap_drv = {
 	.in_use = vfio_ap_mdev_resource_in_use,
 	.on_config_changed = vfio_ap_on_cfg_changed,
 	.on_scan_complete = vfio_ap_on_scan_complete,
+	.on_qstate_transition = vfio_ap_on_qstate_transition,
 	.ids = ap_queue_ids,
 };
 
diff --git a/drivers/s390/crypto/vfio_ap_ops.c b/drivers/s390/crypto/vfio_ap_ops.c
index ffc2d8715bd9..cd4a436c4319 100644
--- a/drivers/s390/crypto/vfio_ap_ops.c
+++ b/drivers/s390/crypto/vfio_ap_ops.c
@@ -841,14 +841,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;
 }
 
 /*
@@ -2809,8 +2809,9 @@ 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));
 	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);
@@ -2902,8 +2903,8 @@ 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);
+	kfree(q);
 }
 
 /**
@@ -3309,3 +3310,46 @@ void vfio_ap_on_scan_complete(struct ap_config_info *new_config_info,
 
 	mutex_unlock(&matrix_dev->guests_lock);
 }
+
+/**
+ * vfio_ap_on_qstate_transition:
+ *
+ * AP bus callback notifying the vfio_ap device driver that the state of a
+ * queue has transitioned.
+ *
+ * @qstate_trans: the object containing a reference to the queue device and the
+ *		  state to which it transitioned.
+ */
+void vfio_ap_on_qstate_transition(struct ap_qstate_transition *qstate_trans)
+{
+	struct vfio_ap_queue *q = vfio_ap_find_queue(qstate_trans->queue->qid);
+	DECLARE_BITMAP(apm_filtered, AP_DEVICES);
+
+	/*
+	 * If the queue is not bound to the vfio_ap device driver, then it won't
+	 * be passed through to a guest; so, no need to continue.
+	 */
+	if (!q)
+		return;
+
+	get_update_locks_for_mdev(q->matrix_mdev);
+
+	switch (qstate_trans->new_state) {
+	case AP_QUEUE_CONFIG_ON:
+	case AP_QUEUE_CHKSTOP_OFF:
+		vfio_ap_mdev_reset_queue(q);
+		flush_work(&q->reset_work);
+
+		if (q->matrix_mdev) {
+			if (vfio_ap_mdev_filter_matrix(q->matrix_mdev, apm_filtered)) {
+				vfio_ap_mdev_update_guest_apcb(q->matrix_mdev);
+				reset_queues_for_apids(q->matrix_mdev, apm_filtered);
+			}
+		}
+		break;
+	default:
+		break;
+	}
+
+	release_update_locks_for_mdev(q->matrix_mdev);
+}
diff --git a/drivers/s390/crypto/vfio_ap_private.h b/drivers/s390/crypto/vfio_ap_private.h
index 9bff666b0b35..c8b00465d258 100644
--- a/drivers/s390/crypto/vfio_ap_private.h
+++ b/drivers/s390/crypto/vfio_ap_private.h
@@ -165,4 +165,6 @@ void vfio_ap_on_cfg_changed(struct ap_config_info *new_config_info,
 void vfio_ap_on_scan_complete(struct ap_config_info *new_config_info,
 			      struct ap_config_info *old_config_info);
 
+void vfio_ap_on_qstate_transition(struct ap_qstate_transition *qstate_trans);
+
 #endif /* _VFIO_AP_PRIVATE_H_ */
-- 
2.53.0

[PATCH v8 6/6] s390/vfio-ap: replace guest-reachable WARNs with ratelimited warnings

[Anthony Krowiak]

WARN and WARN_ONCE macros in code paths reachable by a guest
can be triggered repeatedly by a malicious or misbehaving guest,
flooding the kernel log and potentially impacting system
stability. Replace all WARN and WARN_ONCE calls reachable from
the guest AP interrupt enable/disable and queue reset paths with
ratelimited warning functions. When the queue is assigned to
an mdev, dev_warn_ratelimited() is used so the mdev device name
(which includes the UUID) appears in the message. Otherwise,
pr_warn_ratelimited() is used.

Five reporting functions are introduced:

report_tapq_rc() - reports an invalid or unexpected response
code from PQAP(TAPQ). Used in vfio_ap_wait_for_irqclear() and
apq_status_check(). The signatures of both functions are changed
to accept a struct vfio_ap_queue pointer instead of an apqn so
the queue's mdev context is available for reporting.

report_irqclear_timeout() - reports a timeout waiting for the
IR bit to clear after a PQAP(AQIC) disable in
vfio_ap_wait_for_irqclear().

report_aqic_disable_error() - reports a failed PQAP(AQIC)
disable operation in vfio_ap_irq_disable(). Replaces three
WARN_ONCE calls covering the non-operational queue, rejected
disable, and retry exhaustion cases.

report_zapq_rc() - reports an invalid response code from
PQAP(ZAPQ) in vfio_ap_mdev_reset_queue().

report_gisc_unregister_failure() - reports a failure to
unregister the guest ISC due to the fact that
q->matrix_mdev or q->matrix_mdev->kvm is NULL.

The VFIO_AP_DBF_WARN() calls in vfio_ap_irq_enable() and
handle_pqap() are retained but augmented with companion
dev_warn_ratelimited() or pr_warn_ratelimited() calls.
VFIO_AP_DBF_WARN() writes only to the s390 debug feature
ring buffer, which requires a sysadmin to know to look in
/sys/kernel/debug/s390dbf/ to find the messages. The companion
dmesg log entries ensure that warning conditions are immediately
visible in the kernel log without requiring familiarity with
the s390 debug feature infrastructure.

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

diff --git a/drivers/s390/crypto/vfio_ap_ops.c b/drivers/s390/crypto/vfio_ap_ops.c
index cd4a436c4319..4b6e64daed25 100644
--- a/drivers/s390/crypto/vfio_ap_ops.c
+++ b/drivers/s390/crypto/vfio_ap_ops.c
@@ -226,6 +226,58 @@ static struct vfio_ap_queue *vfio_ap_mdev_get_queue(
 	return NULL;
 }
 
+static void report_tapq_rc(struct vfio_ap_queue *q, u8 rc)
+{
+	if (q->matrix_mdev)
+		dev_warn_ratelimited(mdev_dev(q->matrix_mdev->mdev),
+				     "PQAP(TAPQ) for %02x.%04x failed with invalid rc=%#02x\n",
+				     AP_QID_CARD(q->apqn),
+				     AP_QID_QUEUE(q->apqn), rc);
+	else
+		pr_warn_ratelimited("PQAP(TAPQ) for %02x.%04x failed with invalid rc=%#02x\n",
+				    AP_QID_CARD(q->apqn),
+				    AP_QID_QUEUE(q->apqn), rc);
+}
+
+static void report_irqclear_timeout(struct vfio_ap_queue *q, u8 rc)
+{
+	if (q->matrix_mdev)
+		dev_warn_ratelimited(mdev_dev(q->matrix_mdev->mdev),
+				     "PQAP(TAPQ) timed out waiting for IRQ clear on %02x.%04x: rc=%#02x\n",
+				     AP_QID_CARD(q->apqn),
+				     AP_QID_QUEUE(q->apqn), rc);
+	else
+		pr_warn_ratelimited("PQAP(TAPQ) timed out waiting for IRQ clear on %02x.%04x: rc=%#02x\n",
+				    AP_QID_CARD(q->apqn),
+				    AP_QID_QUEUE(q->apqn), rc);
+}
+
+static void report_aqic_disable_error(struct vfio_ap_queue *q, u8 rc)
+{
+	if (q->matrix_mdev)
+		dev_warn_ratelimited(mdev_dev(q->matrix_mdev->mdev),
+				     "PQAP(AQIC) disable for %02x.%04x failed with rc=%#02x\n",
+				     AP_QID_CARD(q->apqn),
+				     AP_QID_QUEUE(q->apqn), rc);
+	else
+		pr_warn_ratelimited("PQAP(AQIC) disable for %02x.%04x failed with rc=%#02x\n",
+				    AP_QID_CARD(q->apqn),
+				    AP_QID_QUEUE(q->apqn), rc);
+}
+
+static void report_zapq_rc(struct vfio_ap_queue *q, u8 rc)
+{
+	if (q->matrix_mdev)
+		dev_warn_ratelimited(mdev_dev(q->matrix_mdev->mdev),
+				     "PQAP(ZAPQ) for %02x.%04x failed with invalid rc=%#02x\n",
+				     AP_QID_CARD(q->apqn),
+				     AP_QID_QUEUE(q->apqn), rc);
+	else
+		pr_warn_ratelimited("PQAP(ZAPQ) for %02x.%04x failed with invalid rc=%#02x\n",
+				    AP_QID_CARD(q->apqn),
+				    AP_QID_QUEUE(q->apqn), rc);
+}
+
 /**
  * vfio_ap_wait_for_irqclear - wait for the IR bit to clear after a disable
  *
@@ -256,14 +308,16 @@ static struct vfio_ap_queue *vfio_ap_mdev_get_queue(
  *
  * -EIO		PQAP-TAPQ returned an invalid response code
  */
-static int vfio_ap_wait_for_irqclear(int apqn, struct ap_queue_status *tapq_status)
+static int vfio_ap_wait_for_irqclear(struct vfio_ap_queue *q,
+				     struct ap_queue_status *tapq_status)
 {
 	struct ap_queue_status status;
 	int retry = 5;
 
 	do {
-		status = ap_tapq(apqn, NULL);
+		status = ap_tapq(q->apqn, NULL);
 		memcpy(tapq_status, &status, sizeof(status));
+
 		switch (status.response_code) {
 		case AP_RESPONSE_NORMAL:
 		case AP_RESPONSE_RESET_IN_PROGRESS:
@@ -276,8 +330,6 @@ static int vfio_ap_wait_for_irqclear(int apqn, struct ap_queue_status *tapq_stat
 		case AP_RESPONSE_Q_NOT_AVAIL:
 		case AP_RESPONSE_DECONFIGURED:
 		case AP_RESPONSE_CHECKSTOPPED:
-			WARN_ONCE(1, "%s: tapq rc %02x: %04x\n", __func__,
-				  status.response_code, apqn);
 			return -ENODEV;
 		case AP_RESPONSE_ASSOC_SECRET_NOT_UNIQUE:
 		case AP_RESPONSE_ASSOC_FAILED:
@@ -289,23 +341,53 @@ static int vfio_ap_wait_for_irqclear(int apqn, struct ap_queue_status *tapq_stat
 			 * since that would happen anyway if we continued to
 			 * execute the TAPQ.
 			 */
-			WARN_ONCE(1, "%s: tapq rc %02x: %04x\n", __func__,
-				  status.response_code, apqn);
+			report_tapq_rc(q, status.response_code);
 			return -ETIMEDOUT;
 		default:
-			WARN_ONCE(1, "%s: tapq rc %02x: %04x\n", __func__,
-				  status.response_code, apqn);
+			report_tapq_rc(q, status.response_code);
 			return -EIO;
 		}
 	} while (--retry);
 
-	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));
+	report_irqclear_timeout(q, status.response_code);
 
 	return -ETIMEDOUT;
 }
 
+static void report_gisc_unregister_failure(struct vfio_ap_queue *q)
+{
+	if (q->matrix_mdev) {
+		dev_warn_ratelimited(mdev_dev(q->matrix_mdev->mdev),
+				     "APQN %02x.%04x: Failed to unregister guest ISC %c\n",
+				     AP_QID_CARD(q->apqn), AP_QID_QUEUE(q->apqn),
+				     q->saved_isc);
+	} else {
+		pr_warn_ratelimited("APQN %02x.%04x: Failed to unregister guest ISC %c\n",
+				    AP_QID_CARD(q->apqn), AP_QID_QUEUE(q->apqn),
+				    q->saved_isc);
+	}
+}
+
+static bool verify_free_aqic_resourcers(struct vfio_ap_queue *q)
+{
+	bool verified = true;
+
+	if (q->saved_isc != VFIO_AP_ISC_INVALID &&
+	    !(q->matrix_mdev && q->matrix_mdev->kvm)) {
+		report_gisc_unregister_failure(q);
+		verified = false;
+	}
+
+	if (q->saved_iova && !q->matrix_mdev) {
+		pr_warn_ratelimited("APQN %02x.%04x: Failed to unpin NIB page %08x\n",
+				    AP_QID_CARD(q->apqn), AP_QID_QUEUE(q->apqn),
+				    q->saved_iova);
+		verified = false;
+	}
+
+	return verified;
+}
+
 /**
  * vfio_ap_free_aqic_resources - free vfio_ap_queue resources
  * @q: The vfio_ap_queue
@@ -316,17 +398,13 @@ static int vfio_ap_wait_for_irqclear(int apqn, struct ap_queue_status *tapq_stat
  */
 static void vfio_ap_free_aqic_resources(struct vfio_ap_queue *q)
 {
-	if (!q)
+	if (!q || !verify_free_aqic_resourcers(q))
 		return;
-	if (q->saved_isc != VFIO_AP_ISC_INVALID &&
-	    !WARN_ON(!(q->matrix_mdev && q->matrix_mdev->kvm))) {
-		kvm_s390_gisc_unregister(q->matrix_mdev->kvm, q->saved_isc);
-		q->saved_isc = VFIO_AP_ISC_INVALID;
-	}
-	if (q->saved_iova && !WARN_ON(!q->matrix_mdev)) {
-		vfio_unpin_pages(&q->matrix_mdev->vdev, q->saved_iova, 1);
-		q->saved_iova = 0;
-	}
+
+	kvm_s390_gisc_unregister(q->matrix_mdev->kvm, q->saved_isc);
+	q->saved_isc = VFIO_AP_ISC_INVALID;
+	vfio_unpin_pages(&q->matrix_mdev->vdev, q->saved_iova, 1);
+	q->saved_iova = 0;
 }
 
 /**
@@ -373,7 +451,8 @@ static struct ap_queue_status vfio_ap_irq_disable(struct vfio_ap_queue *q)
 			 * wait until interrupt processing has been disabled
 			 * before proceeding.
 			 */
-			ret = vfio_ap_wait_for_irqclear(q->apqn, &tapq_status);
+			ret = vfio_ap_wait_for_irqclear(q, &tapq_status);
+
 			if (ret == 0 || ret == -ENODEV)
 				goto end_free;
 
@@ -420,8 +499,7 @@ static struct ap_queue_status vfio_ap_irq_disable(struct vfio_ap_queue *q)
 		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);
+			report_aqic_disable_error(q, status.response_code);
 			goto end_free;
 		case AP_RESPONSE_INVALID_ADDRESS:
 		case AP_RESPONSE_INVALID_GISA:
@@ -433,14 +511,12 @@ static struct ap_queue_status vfio_ap_irq_disable(struct vfio_ap_queue *q)
 			 * 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);
+			report_aqic_disable_error(q, status.response_code);
 			goto end_fail;
 		}
 	} while (retries--);
 
-	WARN_ONCE(1, "%s: ap_aqic status %d\n", __func__,
-		  status.response_code);
+	report_aqic_disable_error(q, status.response_code);
 
 end_fail:
 	/*
@@ -569,7 +645,10 @@ static struct ap_queue_status vfio_ap_irq_enable(struct vfio_ap_queue *q,
 	if (vfio_ap_validate_nib(vcpu, &nib)) {
 		VFIO_AP_DBF_WARN("%s: invalid NIB address: nib=%pad, apqn=%#04x\n",
 				 __func__, &nib, q->apqn);
-
+		dev_warn_ratelimited(mdev_dev(q->matrix_mdev->mdev),
+				     "PQAP(AQIC) enable for %02x.%04x: invalid NIB address %pad\n",
+				     AP_QID_CARD(q->apqn),
+				     AP_QID_QUEUE(q->apqn), &nib);
 		status.response_code = AP_RESPONSE_INVALID_ADDRESS;
 		return status;
 	}
@@ -584,7 +663,10 @@ static struct ap_queue_status vfio_ap_irq_enable(struct vfio_ap_queue *q,
 		VFIO_AP_DBF_WARN("%s: vfio_pin_pages failed: rc=%d,"
 				 "nib=%pad, apqn=%#04x\n",
 				 __func__, ret, &nib, q->apqn);
-
+		dev_warn_ratelimited(mdev_dev(q->matrix_mdev->mdev),
+				     "PQAP(AQIC) enable for %02x.%04x: vfio_pin_pages failed rc=%d\n",
+				     AP_QID_CARD(q->apqn),
+				     AP_QID_QUEUE(q->apqn), ret);
 		status.response_code = AP_RESPONSE_INVALID_ADDRESS;
 		return status;
 	}
@@ -607,7 +689,10 @@ static struct ap_queue_status vfio_ap_irq_enable(struct vfio_ap_queue *q,
 	if (nisc < 0) {
 		VFIO_AP_DBF_WARN("%s: gisc registration failed: nisc=%d, isc=%d, apqn=%#04x\n",
 				 __func__, nisc, isc, q->apqn);
-
+		dev_warn_ratelimited(mdev_dev(q->matrix_mdev->mdev),
+				     "PQAP(AQIC) enable for %02x.%04x: GISC registration failed rc=%d isc=%d\n",
+				     AP_QID_CARD(q->apqn),
+				     AP_QID_QUEUE(q->apqn), nisc, isc);
 		vfio_unpin_pages(&q->matrix_mdev->vdev, nib, 1);
 		status.response_code = AP_RESPONSE_INVALID_ADDRESS;
 		return status;
@@ -651,9 +736,14 @@ static struct ap_queue_status vfio_ap_irq_enable(struct vfio_ap_queue *q,
 		 * ISC that were prepared for this (rejected) request.
 		 */
 		ret = kvm_s390_gisc_unregister(kvm, isc);
-		if (ret)
+		if (ret) {
 			VFIO_AP_DBF_WARN("%s: kvm_s390_gisc_unregister: rc=%d isc=%d, apqn=%#04x\n",
 					 __func__, ret, isc, q->apqn);
+			dev_warn_ratelimited(mdev_dev(q->matrix_mdev->mdev),
+					     "PQAP(AQIC) enable for %02x.%04x: GISC unregister failed rc=%d isc=%d\n",
+					     AP_QID_CARD(q->apqn),
+					     AP_QID_QUEUE(q->apqn), ret, isc);
+		}
 		vfio_unpin_pages(&q->matrix_mdev->vdev, nib, 1);
 		break;
 	}
@@ -667,6 +757,11 @@ static struct ap_queue_status vfio_ap_irq_enable(struct vfio_ap_queue *q,
 				 aqic_gisa.zone, aqic_gisa.ir, aqic_gisa.gisc,
 				 aqic_gisa.gf, aqic_gisa.gisa, aqic_gisa.isc,
 				 q->apqn);
+		dev_warn_ratelimited(mdev_dev(q->matrix_mdev->mdev),
+				     "PQAP(AQIC) enable for %02x.%04x failed with rc=%#02x\n",
+				     AP_QID_CARD(q->apqn),
+				     AP_QID_QUEUE(q->apqn),
+				     status.response_code);
 	}
 
 	return status;
@@ -751,7 +846,8 @@ static int handle_pqap(struct kvm_vcpu *vcpu)
 	if (!(vcpu->arch.sie_block->eca & ECA_AIV)) {
 		VFIO_AP_DBF_WARN("%s: AIV facility not installed: apqn=0x%04x, eca=0x%04x\n",
 				 __func__, apqn, vcpu->arch.sie_block->eca);
-
+		pr_warn_ratelimited("PQAP(AQIC) for %02x.%04x: AIV facility not installed\n",
+				    AP_QID_CARD(apqn), AP_QID_QUEUE(apqn));
 		return -EOPNOTSUPP;
 	}
 
@@ -760,7 +856,8 @@ static int handle_pqap(struct kvm_vcpu *vcpu)
 	if (!vcpu->kvm->arch.crypto.pqap_hook) {
 		VFIO_AP_DBF_WARN("%s: PQAP(AQIC) hook not registered with the vfio_ap driver: apqn=0x%04x\n",
 				 __func__, apqn);
-
+		pr_warn_ratelimited("PQAP(AQIC) for %02x.%04x: hook not registered with the vfio_ap driver\n",
+				    AP_QID_CARD(apqn), AP_QID_QUEUE(apqn));
 		goto out_unlock;
 	}
 
@@ -773,6 +870,9 @@ static int handle_pqap(struct kvm_vcpu *vcpu)
 		VFIO_AP_DBF_WARN("%s: mdev %08lx-%04lx-%04lx-%04lx-%04lx%08lx not in use: apqn=0x%04x\n",
 				 __func__, uuid[0],  uuid[1], uuid[2],
 				 uuid[3], uuid[4], uuid[5], apqn);
+		dev_warn_ratelimited(mdev_dev(matrix_mdev->mdev),
+				     "PQAP(AQIC) for %02x.%04x: mdev not in use\n",
+				     AP_QID_CARD(apqn), AP_QID_QUEUE(apqn));
 		goto out_unlock;
 	}
 
@@ -781,6 +881,9 @@ static int handle_pqap(struct kvm_vcpu *vcpu)
 		VFIO_AP_DBF_WARN("%s: Queue %02x.%04x not bound to the vfio_ap driver\n",
 				 __func__, AP_QID_CARD(apqn),
 				 AP_QID_QUEUE(apqn));
+		dev_warn_ratelimited(mdev_dev(matrix_mdev->mdev),
+				     "PQAP(AQIC) for %02x.%04x: queue not bound to the vfio_ap driver\n",
+				     AP_QID_CARD(apqn), AP_QID_QUEUE(apqn));
 		goto out_unlock;
 	}
 
@@ -2084,7 +2187,8 @@ static struct vfio_ap_queue *vfio_ap_find_queue(int apqn)
 	return q;
 }
 
-static int apq_status_check(int apqn, struct ap_queue_status *status)
+static int apq_status_check(struct vfio_ap_queue *q,
+			    struct ap_queue_status *status)
 {
 	switch (status->response_code) {
 	case AP_RESPONSE_NORMAL:
@@ -2146,10 +2250,7 @@ static int apq_status_check(int apqn, struct ap_queue_status *status)
 		return -EAGAIN;
 
 	default:
-		WARN(true,
-		     "failed to verify reset of queue %02x.%04x: TAPQ rc=%u\n",
-		     AP_QID_CARD(apqn), AP_QID_QUEUE(apqn),
-		     status->response_code);
+		report_tapq_rc(q, status->response_code);
 		return -EIO;
 	}
 }
@@ -2219,7 +2320,7 @@ static void apq_reset_check(struct work_struct *reset_work)
 		msleep(AP_RESET_INTERVAL);
 		elapsed += AP_RESET_INTERVAL;
 		status = ap_tapq(q->apqn, NULL);
-		ret = apq_status_check(q->apqn, &status);
+		ret = apq_status_check(q, &status);
 		if (ret == -EIO) {
 			/*
 			 * TAPQ returned an invalid response code indicating a
@@ -2343,10 +2444,7 @@ static void vfio_ap_mdev_reset_queue(struct vfio_ap_queue *q)
 		 * would corrupt the new owner's memory which could crash or
 		 * compromise the host kernel.
 		 */
-		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);
+		report_zapq_rc(q, status.response_code);
 	}
 }
 
-- 
2.53.0

Re: [PATCH v8 4/6] 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/20260925124551.665448-1-akrowiak@linux.ibm.com?part=4

Re: [PATCH v8 2/6] 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/20260925124551.665448-1-akrowiak@linux.ibm.com?part=2

Re: [PATCH v8 1/6] s390/vfio-ap: Fix leaks of pinned NIB and registered GISC

[sashiko-bot]

Thank you for your contribution! Sashiko AI review found 1 potential issue(s) to consider:
- [High] s390/vfio-ap: wild DMA write due to unconditional NIB unpin in unmap_iova
--

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

s390/vfio-ap: Fix leaks of pinned NIB and registered GISC

This commit addresses resource leaks of pinned NIB pages and registered
GISCs across several paths. It introduces intentional leaks for cases where
hardware might still be able to write to the NIB, while freeing resources
when queues become unavailable.

> diff --git a/drivers/s390/crypto/vfio_ap_ops.c b/drivers/s390/crypto/vfio_ap_ops.c
> --- a/drivers/s390/crypto/vfio_ap_ops.c
> +++ b/drivers/s390/crypto/vfio_ap_ops.c
[ ... ]
> @@ -1857,8 +2006,28 @@ static void unmap_iova(struct ap_matrix_mdev *matrix_mdev, u64 iova, u64 length)
>  	int loop_cursor;
>  
>  	hash_for_each(qtable->queues, loop_cursor, q, mdev_qnode) {
> -		if (q->saved_iova >= iova && q->saved_iova < iova + length)
> +		if (q->saved_iova >= iova && q->saved_iova < iova + length) {
>  			vfio_ap_irq_disable(q);
> +			/*
> +			 * If IRQ disable failed or IR=0 could not be confirmed,
> +			 * vfio_ap_irq_disable() intentionally leaks the NIB to
> +			 * prevent a wild DMA write. But vfio core requires the
> +			 * page to be unpinned before dma_unmap returns, or it
> +			 * will BUG_ON after 10 re-notification rounds.
> +			 *
> +			 * Fall back to a bounded queue reset. The ZAPQ zeroizes
> +			 * the NIB pointer in hardware, eliminating the DMA risk
> +			 * that justified the leak. Once the worker finishes (or
> +			 * times out with a reset confirmed in-progress), the
> +			 * hardware no longer holds a reference to saved_iova and
> +			 * it is safe to unpin unconditionally.
> +			 */
> +			if (q->saved_iova) {
> +				vfio_ap_mdev_reset_queue(q);
> +				flush_work(&q->reset_work);
> +				vfio_ap_free_aqic_resources(q);

[Severity: High]
Does this unconditional call to vfio_ap_free_aqic_resources() risk allowing
a wild DMA write into host memory?

If vfio_ap_mdev_reset_queue() encounters a hardware fault and TAPQ returns
an invalid response code, it intentionally leaves the NIB pinned. The
comment in vfio_ap_mdev_reset_queue() notes:

"An invalid response code indicates a hardware or firmware bug. Since we
cannot determine whether the queue can still DMA-write to the NIB, the AQIC
resources are intentionally leaked lest the NIB page is reallocated to a new
owner."

However, calling vfio_ap_free_aqic_resources() in unmap_iova() forces the
NIB page to be unpinned regardless of the reset result. If the ZAPQ failed
to zeroize the NIB, unpinning it here overrides the intentional safety leak
and could expose the host to memory corruption.

> +			}
> +		}
>  	}
>  }

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

Re: [PATCH v8 6/6] s390/vfio-ap: replace guest-reachable WARNs with ratelimited warnings

[sashiko-bot]

Thank you for your contribution! Sashiko AI review found 2 potential issue(s) to consider:
- [High] s390/vfio-ap: NULL pointer deref in vfio_ap_free_aqic_resources()
- [Low] s390/vfio-ap: format specifiers truncate DMA addr and corrupt log
--

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

s390/vfio-ap: replace guest-reachable WARNs with ratelimited warnings

WARN and WARN_ONCE macros in code paths reachable by a guest
can be triggered repeatedly by a malicious or misbehaving guest,
flooding the kernel log and potentially impacting system
stability. Replace all WARN and WARN_ONCE calls reachable from
the guest AP interrupt enable/disable and queue reset paths with
ratelimited warning functions.

> diff --git a/drivers/s390/crypto/vfio_ap_ops.c b/drivers/s390/crypto/vfio_ap_ops.c
> index cd4a436c43194..4b6e64daed253 100644
> --- a/drivers/s390/crypto/vfio_ap_ops.c
> +++ b/drivers/s390/crypto/vfio_ap_ops.c
[ ... ]
> +static void report_gisc_unregister_failure(struct vfio_ap_queue *q)
> +{
> +	if (q->matrix_mdev) {
> +		dev_warn_ratelimited(mdev_dev(q->matrix_mdev->mdev),
> +				     "APQN %02x.%04x: Failed to unregister guest ISC %c\n",
> +				     AP_QID_CARD(q->apqn), AP_QID_QUEUE(q->apqn),
> +				     q->saved_isc);

[Severity: Low]
Could this result in missing or mangled information in the kernel log?
The guest ISC is printed using %c, which will output a non-printable ASCII
character for the numeric value, rather than the integer itself.

> +	} else {
> +		pr_warn_ratelimited("APQN %02x.%04x: Failed to unregister guest ISC %c\n",
> +				    AP_QID_CARD(q->apqn), AP_QID_QUEUE(q->apqn),
> +				    q->saved_isc);
> +	}
> +}
> +
> +static bool verify_free_aqic_resourcers(struct vfio_ap_queue *q)
> +{
> +	bool verified = true;
> +
> +	if (q->saved_isc != VFIO_AP_ISC_INVALID &&
> +	    !(q->matrix_mdev && q->matrix_mdev->kvm)) {
> +		report_gisc_unregister_failure(q);
> +		verified = false;
> +	}
> +
> +	if (q->saved_iova && !q->matrix_mdev) {
> +		pr_warn_ratelimited("APQN %02x.%04x: Failed to unpin NIB page %08x\n",
> +				    AP_QID_CARD(q->apqn), AP_QID_QUEUE(q->apqn),
> +				    q->saved_iova);

[Severity: Low]
Does this truncate the logged DMA address? q->saved_iova is a 64-bit 
dma_addr_t, but it's being printed with a 32-bit format specifier %08x.

> +		verified = false;
> +	}
> +
> +	return verified;
> +}
> +
>  /**
>   * vfio_ap_free_aqic_resources - free vfio_ap_queue resources
>   * @q: The vfio_ap_queue
> @@ -316,17 +398,13 @@ static int vfio_ap_wait_for_irqclear(int apqn, struct ap_queue_status *tapq_stat
>   */
>  static void vfio_ap_free_aqic_resources(struct vfio_ap_queue *q)
>  {
> -	if (!q)
> +	if (!q || !verify_free_aqic_resourcers(q))
>  		return;
> -	if (q->saved_isc != VFIO_AP_ISC_INVALID &&
> -	    !WARN_ON(!(q->matrix_mdev && q->matrix_mdev->kvm))) {
> -		kvm_s390_gisc_unregister(q->matrix_mdev->kvm, q->saved_isc);
> -		q->saved_isc = VFIO_AP_ISC_INVALID;
> -	}
> -	if (q->saved_iova && !WARN_ON(!q->matrix_mdev)) {
> -		vfio_unpin_pages(&q->matrix_mdev->vdev, q->saved_iova, 1);
> -		q->saved_iova = 0;
> -	}
> +
> +	kvm_s390_gisc_unregister(q->matrix_mdev->kvm, q->saved_isc);
> +	q->saved_isc = VFIO_AP_ISC_INVALID;
> +	vfio_unpin_pages(&q->matrix_mdev->vdev, q->saved_iova, 1);
> +	q->saved_iova = 0;
>  }

[Severity: High]
Can this lead to a NULL pointer dereference or a memory leak?

In verify_free_aqic_resourcers(), the logic couples the validation of the two
independent resources. If both resources are empty (q->saved_isc == 
VFIO_AP_ISC_INVALID and q->saved_iova == 0), the function returns true. If 
q->matrix_mdev is NULL, vfio_ap_free_aqic_resources() will then unconditionally 
dereference q->matrix_mdev->kvm, resulting in a kernel panic. This path can be 
reached when an unbound queue is removed from the host AP configuration.

Additionally, if only one resource fails validation (e.g., kvm is NULL),
verify_free_aqic_resourcers() returns false, which skips the cleanup entirely.
This prevents the NIB page from being unpinned, leaking the memory.

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

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

[sashiko-bot]

Thank you for your contribution! Sashiko AI review found 6 potential issue(s) to consider:
- [Critical] s390/vfio-ap: unmap_iova() defeats timeout leak, causing wild DMA write
- [Critical] s390/vfio-ap: vfio_ap_mdev_reset_queue() abandons reset on AP_RESPONSE_BUSY
- [Critical] s390/vfio-ap: apq_reset_check() falsely reports zeroization complete
- [High] s390/vfio-ap: apq_reset_check() infinite loop on AP_RESPONSE_BUSY
- [High] s390/vfio-ap: cross-guest resource corruption due to dangling IOVA/ISC
- [Low] s390/vfio-ap: dead code and ineffective state handling on reset timeout
--

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

The commit introduces a 2000ms timeout to apq_reset_check() to prevent
indefinite spinning when the hardware continuously returns a busy response.
If the timeout is reached, the NIB page and ISC registration are intentionally
leaked to avoid memory corruption from potential wild DMA writes. It also
corrects the confirmation logic by checking multiple status bits returned
from TAPQ.

> diff --git a/drivers/s390/crypto/vfio_ap_ops.c b/drivers/s390/crypto/vfio_ap_ops.c
> index 940c0ff668bee..47d4936fb9d7c 100644
> --- a/drivers/s390/crypto/vfio_ap_ops.c
> +++ b/drivers/s390/crypto/vfio_ap_ops.c
[ ... ]
> @@ -1857,8 +2006,28 @@ static void unmap_iova(struct ap_matrix_mdev *matrix_mdev, u64 iova, u64 length)
>  	int loop_cursor;
>  
>  	hash_for_each(qtable->queues, loop_cursor, q, mdev_qnode) {
> -		if (q->saved_iova >= iova && q->saved_iova < iova + length)
> +		if (q->saved_iova >= iova && q->saved_iova < iova + length) {
>  			vfio_ap_irq_disable(q);
> +			/*
> +			 * If IRQ disable failed or IR=0 could not be confirmed,
> +			 * vfio_ap_irq_disable() intentionally leaks the NIB to
> +			 * prevent a wild DMA write. But vfio core requires the
> +			 * page to be unpinned before dma_unmap returns, or it
> +			 * will BUG_ON after 10 re-notification rounds.
> +			 *
> +			 * Fall back to a bounded queue reset. The ZAPQ zeroizes
> +			 * the NIB pointer in hardware, eliminating the DMA risk
> +			 * that justified the leak. Once the worker finishes (or
> +			 * times out with a reset confirmed in-progress), the
> +			 * hardware no longer holds a reference to saved_iova and
> +			 * it is safe to unpin unconditionally.
> +			 */
> +			if (q->saved_iova) {
> +				vfio_ap_mdev_reset_queue(q);
> +				flush_work(&q->reset_work);
> +				vfio_ap_free_aqic_resources(q);

[Severity: Critical]
Could this sequence unconditionally free the AQIC resources immediately after
the timeout completes, defeating the deliberate leak?

apq_reset_check() intentionally leaks the NIB page on timeout because hardware
might still be actively writing to it. However, unmap_iova() executes
flush_work(&q->reset_work) and then unconditionally checks q->saved_iova.

Because the timeout path didn't clear saved_iova, unmap_iova() immediately
unpins the page via vfio_ap_free_aqic_resources(). Returning an active DMA
target to the page allocator could cause host memory corruption via wild DMA
writes.

> +			}
> +		}
>  	}
>  }
[ ... ]
> @@ -1959,8 +2220,77 @@ static void apq_reset_check(struct work_struct *reset_work)
>  		elapsed += AP_RESET_INTERVAL;
>  		status = ap_tapq(q->apqn, NULL);
>  		ret = apq_status_check(q->apqn, &status);
[ ... ]
> +		if (!ret || ret == -ENODEV) {
> +			/*
> +			 * 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.
> +			 */
> +			apq_reset_finalize(q, &status, ret);
> +			return;

[Severity: Critical]
Does this immediate return prevent the rejected ZAPQ instruction from being
reissued, bypassing queue zeroization entirely while falsely reporting success?

If the initial ap_zapq() is rejected (e.g., returns
AP_RESPONSE_STATE_CHANGE_IN_PROGRESS), apq_reset_check() is queued without the
queue being zeroized. apq_reset_check() polls with ap_tapq().

Once the hardware state change finishes, ap_tapq() returns AP_RESPONSE_NORMAL
(queue empty). apq_reset_check() sees !ret, executes apq_reset_finalize(),
and returns success.

The ZAPQ command is never reissued. The queue is falsely marked as reset and
can be assigned to a new guest with the previous guest's cryptographic secrets
intact.

> +		}
> +
> +		if (elapsed >= AP_RESET_MAX_WAIT) {
> +			/*
> +			 * Timed out without being able to verify zapq completed.
[ ... ]
> +			 */
> +			report_aqic_resource_leak(q);
> +			/*
> +			 * Zeroization could not be confirmed; set
> +			 * reset_status to AP_RESPONSE_RESET_IN_PROGRESS.
> +			 * This is used internally to signal that the reset
> +			 * did not complete, and ensures that if the queue
> +			 * is reset again, the re-issue logic in
> +			 * apq_reset_check() will re-issue the ZAPQ.
> +			 */
> +			q->reset_status.response_code = AP_RESPONSE_RESET_IN_PROGRESS;
> +
> +			return;

[Severity: High]
Can leaving q->saved_iova and q->saved_isc populated after the timeout corrupt
cross-guest resources?

If the queue is assigned to a new guest, q->matrix_mdev points to the new
guest's matrix_mdev. When the new guest enables interrupts,
vfio_ap_free_aqic_resources() is called.

It uses the new q->matrix_mdev to unpin the old q->saved_iova and unregister
the old q->saved_isc, corrupting the new guest's IOMMU pinning counts
and KVM ISC refcounts.

> +		}
> +
>  		if (ret == -EBUSY) {
>  			pr_notice_ratelimited(WAIT_MSG, elapsed,
>  					      AP_QID_CARD(q->apqn),
> @@ -1968,18 +2298,15 @@ static void apq_reset_check(struct work_struct *reset_work)
>  					      status.response_code,
>  					      status.queue_empty,
>  					      status.irq_enabled);
> -		} else {
> -			if (q->reset_status.response_code == AP_RESPONSE_RESET_IN_PROGRESS ||
> -			    q->reset_status.response_code == AP_RESPONSE_BUSY ||
> -			    q->reset_status.response_code == AP_RESPONSE_STATE_CHANGE_IN_PROGRESS ||
> -			    ret == -EAGAIN) {
> -				status = ap_zapq(q->apqn, 0);
> -				memcpy(&q->reset_status, &status, sizeof(status));
> -				continue;
> -			}
> -			if (q->saved_isc != VFIO_AP_ISC_INVALID)
> -				vfio_ap_free_aqic_resources(q);
> -			break;
> +			continue;
> +		}
> +
> +		if (ret == -EAGAIN ||
> +		    q->reset_status.response_code == AP_RESPONSE_RESET_IN_PROGRESS ||
> +		    q->reset_status.response_code == AP_RESPONSE_STATE_CHANGE_IN_PROGRESS) {

[Severity: Low]
Is this condition checking dead code because earlier flow control guarantees
ret == -EAGAIN is always true here?

The code attempts to signal a failed reset by setting
reset_status.response_code = AP_RESPONSE_RESET_IN_PROGRESS on timeout.

However, vfio_ap_mdev_reset_queue() overwrites reset_status unconditionally
with the result of a new ap_zapq() call on the next reset attempt, rendering
the signal useless. Furthermore, the conditional check for this status inside
apq_reset_check() is completely unreachable because execution only reaches that
block if ret == -EAGAIN, which short-circuits the OR condition.

> +			status = ap_zapq(q->apqn, 0);
> +			memcpy(&q->reset_status, &status, sizeof(status));
> +			elapsed = 0;

[Severity: High]
Will this repeatedly reset the timeout counter when dealing with busy
responses, bypassing the timeout logic?

If the hardware continuously returns AP_RESPONSE_BUSY, apq_status_check()
maps this to -EAGAIN. In apq_reset_check(), if ret == -EAGAIN, the elapsed
timeout counter is reset to 0, completely bypassing the new 2000ms timeout
logic.

This causes the worker to spin indefinitely, deadlocking the host system as
callers hold global mutexes like mdevs_lock while waiting on flush_work().

>  		}
>  	}
>  }
[ ... ]
> @@ -1995,18 +2322,27 @@ static void vfio_ap_mdev_reset_queue(struct vfio_ap_queue *q)
>  	switch (status.response_code) {
>  	case AP_RESPONSE_NORMAL:
>  	case AP_RESPONSE_RESET_IN_PROGRESS:
> -	case AP_RESPONSE_BUSY:
>  	case AP_RESPONSE_STATE_CHANGE_IN_PROGRESS:
>  		/*
>  		 * Let's verify whether the ZAPQ completed successfully on a work queue.
>  		 */
>  		queue_work(system_long_wq, &q->reset_work);
>  		break;

[Severity: Critical]
Would removing AP_RESPONSE_BUSY here cause the flow to fall through to the
default error case?

If the initial ap_zapq() returns AP_RESPONSE_BUSY, it falls to the default
case, aborting the reset and not queueing the reset_work.

This leaves the cryptographic queue unzeroized, leading to an information
leak when the queue is reassigned.

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

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

[sashiko-bot]

Thank you for your contribution! Sashiko AI review found 3 potential issue(s) to consider:
- [High] crypto: vfio: Use-After-Free of vfio_ap_queue in vfio_ap_on_qstate_transition
- [High] crypto: vfio: TOCTOU on q->matrix_mdev leads to unprotected KVM state updates and deadlocks
- [Low] crypto: vfio: Incorrect comment for __drv_notify_qstate_transitioned
--

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

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

This patch limits the wait for a queue reset to complete and prevents the
leakage of queue state to a guest. It introduces an on_qstate_transition
callback to track queue state changes and restricts queue passability to
fully zeroized states.

Cc: stable@vger.kernel.org

> diff --git a/drivers/s390/crypto/ap_bus.c b/drivers/s390/crypto/ap_bus.c
> index d82df5b4e2db3..a53cfad3543eb 100644
> --- a/drivers/s390/crypto/ap_bus.c
> +++ b/drivers/s390/crypto/ap_bus.c
[ ... ]
> @@ -1979,6 +1979,28 @@ static inline void notify_scan_complete(void)
>  			 __drv_notify_scan_complete);
>  }
>  
> +/* Helper function for notify_config_changed */
> +static int __drv_notify_qstate_transitioned(struct device_driver *drv, void *data)

[Severity: Low]
Is this comment accurate? It states this is a helper function for
notify_config_changed, but the function name and usage indicate it is a
helper for notify_qstate_transitioned.

> +{
[ ... ]
> diff --git a/drivers/s390/crypto/vfio_ap_ops.c b/drivers/s390/crypto/vfio_ap_ops.c
> index ffc2d8715bd96..cd4a436c43194 100644
> --- a/drivers/s390/crypto/vfio_ap_ops.c
> +++ b/drivers/s390/crypto/vfio_ap_ops.c
[ ... ]
> @@ -3309,3 +3310,46 @@ void vfio_ap_on_scan_complete(struct ap_config_info *new_config_info,
>  
>  	mutex_unlock(&matrix_dev->guests_lock);
>  }
> +
> +/**
> + * vfio_ap_on_qstate_transition:
> + *
> + * AP bus callback notifying the vfio_ap device driver that the state of a
> + * queue has transitioned.
> + *
> + * @qstate_trans: the object containing a reference to the queue device and the
> + *		  state to which it transitioned.
> + */
> +void vfio_ap_on_qstate_transition(struct ap_qstate_transition *qstate_trans)
> +{
> +	struct vfio_ap_queue *q = vfio_ap_find_queue(qstate_trans->queue->qid);
> +	DECLARE_BITMAP(apm_filtered, AP_DEVICES);
> +
> +	/*
> +	 * If the queue is not bound to the vfio_ap device driver, then it won't
> +	 * be passed through to a guest; so, no need to continue.
> +	 */
> +	if (!q)
> +		return;
> +
> +	get_update_locks_for_mdev(q->matrix_mdev);

[Severity: High]
Can this dereference a freed pointer?

If vfio_ap_find_queue() reads the device's drvdata and drops the device
reference before returning, q has no reference count. Because
vfio_ap_on_qstate_transition() holds no locks, a concurrent sysfs unbind
can invoke vfio_ap_mdev_remove_queue(), which clears drvdata and calls
kfree(q).

This would cause a use-after-free when reading q->matrix_mdev.

Additionally, is there a TOCTOU race when reading q->matrix_mdev without
locks?

If q->matrix_mdev is NULL here, get_update_locks_for_mdev() will skip
acquiring kvm->lock. If another thread concurrently updates q->matrix_mdev
to a non-NULL value while waiting on guests_lock, this could lead to issues
later in the function.

> +
> +	switch (qstate_trans->new_state) {
> +	case AP_QUEUE_CONFIG_ON:
> +	case AP_QUEUE_CHKSTOP_OFF:
> +		vfio_ap_mdev_reset_queue(q);
> +		flush_work(&q->reset_work);

[Severity: High]
Does this also cause a use-after-free for the same reason? Passing the
potentially freed queue object to vfio_ap_mdev_reset_queue(q) and flushing
its work struct could trigger further memory corruption.

> +
> +		if (q->matrix_mdev) {
> +			if (vfio_ap_mdev_filter_matrix(q->matrix_mdev, apm_filtered)) {
> +				vfio_ap_mdev_update_guest_apcb(q->matrix_mdev);

[Severity: High]
If q->matrix_mdev changed concurrently after get_update_locks_for_mdev()
was called, could this update the KVM guest state without having acquired
the proper KVM locks?

> +				reset_queues_for_apids(q->matrix_mdev, apm_filtered);
> +			}
> +		}
> +		break;
> +	default:
> +		break;
> +	}
> +
> +	release_update_locks_for_mdev(q->matrix_mdev);

[Severity: High]
If q->matrix_mdev was modified between the initial acquisition and here,
would this call mutex_unlock() on a lock it did not acquire, or leave
an acquired lock perpetually locked?

> +}

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

Re: [PATCH v8 6/6] s390/vfio-ap: replace guest-reachable WARNs with ratelimited warnings

[Anthony Krowiak]

On 9/25/26 8:59 AM, sashiko-bot@kernel.org wrote:
> Thank you for your contribution! Sashiko AI review found 2 potential issue(s) to consider:
> - [High] s390/vfio-ap: NULL pointer deref in vfio_ap_free_aqic_resources()
> - [Low] s390/vfio-ap: format specifiers truncate DMA addr and corrupt log
> --
>
> commit bb0d1a9a828269662641d7a1b6f445a55a9d0e1d
> Author: Anthony Krowiak <akrowiak@linux.ibm.com>
>
> s390/vfio-ap: replace guest-reachable WARNs with ratelimited warnings
>
> WARN and WARN_ONCE macros in code paths reachable by a guest
> can be triggered repeatedly by a malicious or misbehaving guest,
> flooding the kernel log and potentially impacting system
> stability. Replace all WARN and WARN_ONCE calls reachable from
> the guest AP interrupt enable/disable and queue reset paths with
> ratelimited warning functions.
>
>> diff --git a/drivers/s390/crypto/vfio_ap_ops.c b/drivers/s390/crypto/vfio_ap_ops.c
>> index cd4a436c43194..4b6e64daed253 100644
>> --- a/drivers/s390/crypto/vfio_ap_ops.c
>> +++ b/drivers/s390/crypto/vfio_ap_ops.c
> [ ... ]
>> +static void report_gisc_unregister_failure(struct vfio_ap_queue *q)
>> +{
>> +	if (q->matrix_mdev) {
>> +		dev_warn_ratelimited(mdev_dev(q->matrix_mdev->mdev),
>> +				     "APQN %02x.%04x: Failed to unregister guest ISC %c\n",
>> +				     AP_QID_CARD(q->apqn), AP_QID_QUEUE(q->apqn),
>> +				     q->saved_isc);
> [Severity: Low]
> Could this result in missing or mangled information in the kernel log?
> The guest ISC is printed using %c, which will output a non-printable ASCII
> character for the numeric value, rather than the integer itself.

Will change to %d

>
>> +	} else {
>> +		pr_warn_ratelimited("APQN %02x.%04x: Failed to unregister guest ISC %c\n",
>> +				    AP_QID_CARD(q->apqn), AP_QID_QUEUE(q->apqn),
>> +				    q->saved_isc);
>> +	}
>> +}

Same here

>> +
>> +static bool verify_free_aqic_resourcers(struct vfio_ap_queue *q)
>> +{
>> +	bool verified = true;
>> +
>> +	if (q->saved_isc != VFIO_AP_ISC_INVALID &&
>> +	    !(q->matrix_mdev && q->matrix_mdev->kvm)) {
>> +		report_gisc_unregister_failure(q);
>> +		verified = false;
>> +	}
>> +
>> +	if (q->saved_iova && !q->matrix_mdev) {
>> +		pr_warn_ratelimited("APQN %02x.%04x: Failed to unpin NIB page %08x\n",
>> +				    AP_QID_CARD(q->apqn), AP_QID_QUEUE(q->apqn),
>> +				    q->saved_iova);
> [Severity: Low]
> Does this truncate the logged DMA address? q->saved_iova is a 64-bit
> dma_addr_t, but it's being printed with a 32-bit format specifier %08x.

Will change to %pad

>
>> +		verified = false;
>> +	}
>> +
>> +	return verified;
>> +}
>> +
>>   /**
>>    * vfio_ap_free_aqic_resources - free vfio_ap_queue resources
>>    * @q: The vfio_ap_queue
>> @@ -316,17 +398,13 @@ static int vfio_ap_wait_for_irqclear(int apqn, struct ap_queue_status *tapq_stat
>>    */
>>   static void vfio_ap_free_aqic_resources(struct vfio_ap_queue *q)
>>   {
>> -	if (!q)
>> +	if (!q || !verify_free_aqic_resourcers(q))
>>   		return;
>> -	if (q->saved_isc != VFIO_AP_ISC_INVALID &&
>> -	    !WARN_ON(!(q->matrix_mdev && q->matrix_mdev->kvm))) {
>> -		kvm_s390_gisc_unregister(q->matrix_mdev->kvm, q->saved_isc);
>> -		q->saved_isc = VFIO_AP_ISC_INVALID;
>> -	}
>> -	if (q->saved_iova && !WARN_ON(!q->matrix_mdev)) {
>> -		vfio_unpin_pages(&q->matrix_mdev->vdev, q->saved_iova, 1);
>> -		q->saved_iova = 0;
>> -	}
>> +
>> +	kvm_s390_gisc_unregister(q->matrix_mdev->kvm, q->saved_isc);
>> +	q->saved_isc = VFIO_AP_ISC_INVALID;
>> +	vfio_unpin_pages(&q->matrix_mdev->vdev, q->saved_iova, 1);
>> +	q->saved_iova = 0;
>>   }
> [Severity: High]
> Can this lead to a NULL pointer dereference or a memory leak?
>
> In verify_free_aqic_resourcers(), the logic couples the validation of the two
> independent resources. If both resources are empty (q->saved_isc ==
> VFIO_AP_ISC_INVALID and q->saved_iova == 0), the function returns true. If
> q->matrix_mdev is NULL, vfio_ap_free_aqic_resources() will then unconditionally
> dereference q->matrix_mdev->kvm, resulting in a kernel panic. This path can be
> reached when an unbound queue is removed from the host AP configuration.
>
> Additionally, if only one resource fails validation (e.g., kvm is NULL),
> verify_free_aqic_resourcers() returns false, which skips the cleanup entirely.
> This prevents the NIB page from being unpinned, leaking the memory.

Getting rid of verify_free_aqic_resourcers() and doing the checking in 
vfio_ap_free_aqic_resources() with a fix that eliminates the NULL 
dereference.
>

Re: [PATCH v8 1/6] s390/vfio-ap: Fix leaks of pinned NIB and registered GISC

[Anthony Krowiak]

On 9/25/26 8:57 AM, sashiko-bot@kernel.org wrote:
> Thank you for your contribution! Sashiko AI review found 1 potential issue(s) to consider:
> - [High] s390/vfio-ap: wild DMA write due to unconditional NIB unpin in unmap_iova
> --
>
> commit f138c70af93258555b6e1c1c1233b5d5dc8b82da
> Author: Anthony Krowiak <akrowiak@linux.ibm.com>
>
> s390/vfio-ap: Fix leaks of pinned NIB and registered GISC
>
> This commit addresses resource leaks of pinned NIB pages and registered
> GISCs across several paths. It introduces intentional leaks for cases where
> hardware might still be able to write to the NIB, while freeing resources
> when queues become unavailable.
>
>> diff --git a/drivers/s390/crypto/vfio_ap_ops.c b/drivers/s390/crypto/vfio_ap_ops.c
>> --- a/drivers/s390/crypto/vfio_ap_ops.c
>> +++ b/drivers/s390/crypto/vfio_ap_ops.c
> [ ... ]
>> @@ -1857,8 +2006,28 @@ static void unmap_iova(struct ap_matrix_mdev *matrix_mdev, u64 iova, u64 length)
>>   	int loop_cursor;
>>   
>>   	hash_for_each(qtable->queues, loop_cursor, q, mdev_qnode) {
>> -		if (q->saved_iova >= iova && q->saved_iova < iova + length)
>> +		if (q->saved_iova >= iova && q->saved_iova < iova + length) {
>>   			vfio_ap_irq_disable(q);
>> +			/*
>> +			 * If IRQ disable failed or IR=0 could not be confirmed,
>> +			 * vfio_ap_irq_disable() intentionally leaks the NIB to
>> +			 * prevent a wild DMA write. But vfio core requires the
>> +			 * page to be unpinned before dma_unmap returns, or it
>> +			 * will BUG_ON after 10 re-notification rounds.
>> +			 *
>> +			 * Fall back to a bounded queue reset. The ZAPQ zeroizes
>> +			 * the NIB pointer in hardware, eliminating the DMA risk
>> +			 * that justified the leak. Once the worker finishes (or
>> +			 * times out with a reset confirmed in-progress), the
>> +			 * hardware no longer holds a reference to saved_iova and
>> +			 * it is safe to unpin unconditionally.
>> +			 */
>> +			if (q->saved_iova) {
>> +				vfio_ap_mdev_reset_queue(q);
>> +				flush_work(&q->reset_work);
>> +				vfio_ap_free_aqic_resources(q);
> [Severity: High]
> Does this unconditional call to vfio_ap_free_aqic_resources() risk allowing
> a wild DMA write into host memory?
>
> If vfio_ap_mdev_reset_queue() encounters a hardware fault and TAPQ returns
> an invalid response code, it intentionally leaves the NIB pinned. The
> comment in vfio_ap_mdev_reset_queue() notes:
>
> "An invalid response code indicates a hardware or firmware bug. Since we
> cannot determine whether the queue can still DMA-write to the NIB, the AQIC
> resources are intentionally leaked lest the NIB page is reallocated to a new
> owner."
>
> However, calling vfio_ap_free_aqic_resources() in unmap_iova() forces the
> NIB page to be unpinned regardless of the reset result. If the ZAPQ failed
> to zeroize the NIB, unpinning it here overrides the intentional safety leak
> and could expose the host to memory corruption.

vfio_ap_reset_queue() along with the apq_reset_check() work queue function
frees the AQIC resources only if reset completion is verified; so, the
freeing of resources should be delegated to vfio_ap_reset_queue()
and not unconditionally.

>
>> +			}
>> +		}
>>   	}
>>   }

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

[Anthony Krowiak]

On 9/25/26 9:00 AM, sashiko-bot@kernel.org wrote:
> Thank you for your contribution! Sashiko AI review found 6 potential issue(s) to consider:
> - [Critical] s390/vfio-ap: unmap_iova() defeats timeout leak, causing wild DMA write
> - [Critical] s390/vfio-ap: vfio_ap_mdev_reset_queue() abandons reset on AP_RESPONSE_BUSY
> - [Critical] s390/vfio-ap: apq_reset_check() falsely reports zeroization complete
> - [High] s390/vfio-ap: apq_reset_check() infinite loop on AP_RESPONSE_BUSY
> - [High] s390/vfio-ap: cross-guest resource corruption due to dangling IOVA/ISC
> - [Low] s390/vfio-ap: dead code and ineffective state handling on reset timeout
> --
>
> commit 3205ec637b4cfa8d70d28a8f5abc909aba0bd19c
> Author: Anthony Krowiak <akrowiak@linux.ibm.com>
> Subject: s390/vfio-ap: Fix unbounded loop in apq_reset_check()
>
> The commit introduces a 2000ms timeout to apq_reset_check() to prevent
> indefinite spinning when the hardware continuously returns a busy response.
> If the timeout is reached, the NIB page and ISC registration are intentionally
> leaked to avoid memory corruption from potential wild DMA writes. It also
> corrects the confirmation logic by checking multiple status bits returned
> from TAPQ.
>
>> diff --git a/drivers/s390/crypto/vfio_ap_ops.c b/drivers/s390/crypto/vfio_ap_ops.c
>> index 940c0ff668bee..47d4936fb9d7c 100644
>> --- a/drivers/s390/crypto/vfio_ap_ops.c
>> +++ b/drivers/s390/crypto/vfio_ap_ops.c
> [ ... ]
>> @@ -1857,8 +2006,28 @@ static void unmap_iova(struct ap_matrix_mdev *matrix_mdev, u64 iova, u64 length)
>>   	int loop_cursor;
>>   
>>   	hash_for_each(qtable->queues, loop_cursor, q, mdev_qnode) {
>> -		if (q->saved_iova >= iova && q->saved_iova < iova + length)
>> +		if (q->saved_iova >= iova && q->saved_iova < iova + length) {
>>   			vfio_ap_irq_disable(q);
>> +			/*
>> +			 * If IRQ disable failed or IR=0 could not be confirmed,
>> +			 * vfio_ap_irq_disable() intentionally leaks the NIB to
>> +			 * prevent a wild DMA write. But vfio core requires the
>> +			 * page to be unpinned before dma_unmap returns, or it
>> +			 * will BUG_ON after 10 re-notification rounds.
>> +			 *
>> +			 * Fall back to a bounded queue reset. The ZAPQ zeroizes
>> +			 * the NIB pointer in hardware, eliminating the DMA risk
>> +			 * that justified the leak. Once the worker finishes (or
>> +			 * times out with a reset confirmed in-progress), the
>> +			 * hardware no longer holds a reference to saved_iova and
>> +			 * it is safe to unpin unconditionally.
>> +			 */
>> +			if (q->saved_iova) {
>> +				vfio_ap_mdev_reset_queue(q);
>> +				flush_work(&q->reset_work);
>> +				vfio_ap_free_aqic_resources(q);
> [Severity: Critical]
> Could this sequence unconditionally free the AQIC resources immediately after
> the timeout completes, defeating the deliberate leak?
>
> apq_reset_check() intentionally leaks the NIB page on timeout because hardware
> might still be actively writing to it. However, unmap_iova() executes
> flush_work(&q->reset_work) and then unconditionally checks q->saved_iova.
>
> Because the timeout path didn't clear saved_iova, unmap_iova() immediately
> unpins the page via vfio_ap_free_aqic_resources(). Returning an active DMA
> target to the page allocator could cause host memory corruption via wild DMA
> writes.

Asked and answered in response to same comment in patch 1/6.

>
>> +			}
>> +		}
>>   	}
>>   }
> [ ... ]
>> @@ -1959,8 +2220,77 @@ static void apq_reset_check(struct work_struct *reset_work)
>>   		elapsed += AP_RESET_INTERVAL;
>>   		status = ap_tapq(q->apqn, NULL);
>>   		ret = apq_status_check(q->apqn, &status);
> [ ... ]
>> +		if (!ret || ret == -ENODEV) {
>> +			/*
>> +			 * 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.
>> +			 */
>> +			apq_reset_finalize(q, &status, ret);
>> +			return;
> [Severity: Critical]
> Does this immediate return prevent the rejected ZAPQ instruction from being
> reissued, bypassing queue zeroization entirely while falsely reporting success?
>
> If the initial ap_zapq() is rejected (e.g., returns
> AP_RESPONSE_STATE_CHANGE_IN_PROGRESS), apq_reset_check() is queued without the
> queue being zeroized. apq_reset_check() polls with ap_tapq().
>
> Once the hardware state change finishes, ap_tapq() returns AP_RESPONSE_NORMAL
> (queue empty). apq_reset_check() sees !ret, executes apq_reset_finalize(),
> and returns success.
>
> The ZAPQ command is never reissued. The queue is falsely marked as reset and
> can be assigned to a new guest with the previous guest's cryptographic secrets
> intact.

This is a legitimate concern and will be fixed in v9 of the series 
forthcoming.

>
>> +		}
>> +
>> +		if (elapsed >= AP_RESET_MAX_WAIT) {
>> +			/*
>> +			 * Timed out without being able to verify zapq completed.
> [ ... ]
>> +			 */
>> +			report_aqic_resource_leak(q);
>> +			/*
>> +			 * Zeroization could not be confirmed; set
>> +			 * reset_status to AP_RESPONSE_RESET_IN_PROGRESS.
>> +			 * This is used internally to signal that the reset
>> +			 * did not complete, and ensures that if the queue
>> +			 * is reset again, the re-issue logic in
>> +			 * apq_reset_check() will re-issue the ZAPQ.
>> +			 */
>> +			q->reset_status.response_code = AP_RESPONSE_RESET_IN_PROGRESS;
>> +
>> +			return;
> [Severity: High]
> Can leaving q->saved_iova and q->saved_isc populated after the timeout corrupt
> cross-guest resources?
>
> If the queue is assigned to a new guest, q->matrix_mdev points to the new
> guest's matrix_mdev. When the new guest enables interrupts,
> vfio_ap_free_aqic_resources() is called.
>
> It uses the new q->matrix_mdev to unpin the old q->saved_iova and unregister
> the old q->saved_isc, corrupting the new guest's IOMMU pinning counts
> and KVM ISC refcounts.

This is a legitimate concern; The guest ISC needs to be unregistered 
using the
kvm pointer used to register it if possible. The pinned NIB page needs to be
freed using the vfio_device whose IOMMU container owns the pinned NIB
page. This will be fixed in v9 of the series forthcoming.

>
>> +		}
>> +
>>   		if (ret == -EBUSY) {
>>   			pr_notice_ratelimited(WAIT_MSG, elapsed,
>>   					      AP_QID_CARD(q->apqn),
>> @@ -1968,18 +2298,15 @@ static void apq_reset_check(struct work_struct *reset_work)
>>   					      status.response_code,
>>   					      status.queue_empty,
>>   					      status.irq_enabled);
>> -		} else {
>> -			if (q->reset_status.response_code == AP_RESPONSE_RESET_IN_PROGRESS ||
>> -			    q->reset_status.response_code == AP_RESPONSE_BUSY ||
>> -			    q->reset_status.response_code == AP_RESPONSE_STATE_CHANGE_IN_PROGRESS ||
>> -			    ret == -EAGAIN) {
>> -				status = ap_zapq(q->apqn, 0);
>> -				memcpy(&q->reset_status, &status, sizeof(status));
>> -				continue;
>> -			}
>> -			if (q->saved_isc != VFIO_AP_ISC_INVALID)
>> -				vfio_ap_free_aqic_resources(q);
>> -			break;
>> +			continue;
>> +		}
>> +
>> +		if (ret == -EAGAIN ||
>> +		    q->reset_status.response_code == AP_RESPONSE_RESET_IN_PROGRESS ||
>> +		    q->reset_status.response_code == AP_RESPONSE_STATE_CHANGE_IN_PROGRESS) {
> [Severity: Low]
> Is this condition checking dead code because earlier flow control guarantees
> ret == -EAGAIN is always true here?
>
> The code attempts to signal a failed reset by setting
> reset_status.response_code = AP_RESPONSE_RESET_IN_PROGRESS on timeout.
>
> However, vfio_ap_mdev_reset_queue() overwrites reset_status unconditionally
> with the result of a new ap_zapq() call on the next reset attempt, rendering
> the signal useless. Furthermore, the conditional check for this status inside
> apq_reset_check() is completely unreachable because execution only reaches that
> block if ret == -EAGAIN, which short-circuits the OR condition.

The premature early exit for !ret needs to be removed from the top of 
the loop.
When the worker starts after an initial ap_zapq() returns
AP_RESPONSE_STATE_CHANGE_IN_PROGRESS or AP_RESPONSE_RESET_IN_PROGRESS,
TAPQ might initially report AP_RESPONSE_NORMAL (returning ret = 0); in 
this case,
the queue will not have been reset/zeroized, so we need to let it fall 
through to
the block above. This will be fixed in the forthcoming v9 of the series.


>
>> +			status = ap_zapq(q->apqn, 0);
>> +			memcpy(&q->reset_status, &status, sizeof(status));
>> +			elapsed = 0;
> [Severity: High]
> Will this repeatedly reset the timeout counter when dealing with busy
> responses, bypassing the timeout logic?
>
> If the hardware continuously returns AP_RESPONSE_BUSY, apq_status_check()
> maps this to -EAGAIN. In apq_reset_check(), if ret == -EAGAIN, the elapsed
> timeout counter is reset to 0, completely bypassing the new 2000ms timeout
> logic.
>
> This causes the worker to spin indefinitely, deadlocking the host system as
> callers hold global mutexes like mdevs_lock while waiting on flush_work().

The the 'elapsed' timeout counter should not be reset to 0.
If the hardware continuously returns busy / EAGAIN responses, the
loop will be guaranteed to exit once elapsed >= AP_RESET_MAX_WAIT (2000ms),
preventing any unbounded spinning or host deadlock.

>
>>   		}
>>   	}
>>   }
> [ ... ]
>> @@ -1995,18 +2322,27 @@ static void vfio_ap_mdev_reset_queue(struct vfio_ap_queue *q)
>>   	switch (status.response_code) {
>>   	case AP_RESPONSE_NORMAL:
>>   	case AP_RESPONSE_RESET_IN_PROGRESS:
>> -	case AP_RESPONSE_BUSY:
>>   	case AP_RESPONSE_STATE_CHANGE_IN_PROGRESS:
>>   		/*
>>   		 * Let's verify whether the ZAPQ completed successfully on a work queue.
>>   		 */
>>   		queue_work(system_long_wq, &q->reset_work);
>>   		break;
> [Severity: Critical]
> Would removing AP_RESPONSE_BUSY here cause the flow to fall through to the
> default error case?
>
> If the initial ap_zapq() returns AP_RESPONSE_BUSY, it falls to the default
> case, aborting the reset and not queueing the reset_work.
>
> This leaves the cryptographic queue unzeroized, leading to an information
> leak when the queue is reassigned.

The AP_RESPONSE_BUSY (05) response code is not a valid response for
ZAPQ according to the architecture; so, it makes sense to let it fall 
through
to the default case since it is architecturally incorrect.  On the other 
hand,
AP_RESPONSE_BUSY is valid for TAPQ which is used by apq_status_check()
to verify whether a reset has successfully completed. If TAPQ returns
AP_RESPONSE_BUSY, the status check returns -EAGAIN so the ZAPQ
is re-issued.

>

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

[Anthony Krowiak]

On 9/25/26 9:03 AM, sashiko-bot@kernel.org wrote:
> Thank you for your contribution! Sashiko AI review found 3 potential issue(s) to consider:
> - [High] crypto: vfio: Use-After-Free of vfio_ap_queue in vfio_ap_on_qstate_transition
> - [High] crypto: vfio: TOCTOU on q->matrix_mdev leads to unprotected KVM state updates and deadlocks
> - [Low] crypto: vfio: Incorrect comment for __drv_notify_qstate_transitioned
> --
>
> commit 6964322bafaab43ec603a04a262b5d1370b6779d
> Author: Anthony Krowiak <akrowiak@linux.ibm.com>
>
> s390/vfio-ap: fix queue state leakage to guest and host
>
> This patch limits the wait for a queue reset to complete and prevents the
> leakage of queue state to a guest. It introduces an on_qstate_transition
> callback to track queue state changes and restricts queue passability to
> fully zeroized states.
>
> Cc: stable@vger.kernel.org
>
>> diff --git a/drivers/s390/crypto/ap_bus.c b/drivers/s390/crypto/ap_bus.c
>> index d82df5b4e2db3..a53cfad3543eb 100644
>> --- a/drivers/s390/crypto/ap_bus.c
>> +++ b/drivers/s390/crypto/ap_bus.c
> [ ... ]
>> @@ -1979,6 +1979,28 @@ static inline void notify_scan_complete(void)
>>   			 __drv_notify_scan_complete);
>>   }
>>   
>> +/* Helper function for notify_config_changed */
>> +static int __drv_notify_qstate_transitioned(struct device_driver *drv, void *data)
> [Severity: Low]
> Is this comment accurate? It states this is a helper function for
> notify_config_changed, but the function name and usage indicate it is a
> helper for notify_qstate_transitioned.

This will be fixed ion the forthcoming v9 patch series

>
>> +{
> [ ... ]
>> diff --git a/drivers/s390/crypto/vfio_ap_ops.c b/drivers/s390/crypto/vfio_ap_ops.c
>> index ffc2d8715bd96..cd4a436c43194 100644
>> --- a/drivers/s390/crypto/vfio_ap_ops.c
>> +++ b/drivers/s390/crypto/vfio_ap_ops.c
> [ ... ]
>> @@ -3309,3 +3310,46 @@ void vfio_ap_on_scan_complete(struct ap_config_info *new_config_info,
>>   
>>   	mutex_unlock(&matrix_dev->guests_lock);
>>   }
>> +
>> +/**
>> + * vfio_ap_on_qstate_transition:
>> + *Instead of calling vfio_ap_find_queue() without locks and reading q->matrix_mdev, vfio_ap_on_qstate_transition() now calls get_update_locks_by_apqn(apqn)
>> + * AP bus callback notifying the vfio_ap device driver that the state of a
>> + * queue has transitioned.
>> + *
>> + * @qstate_trans: the object containing a reference to the queue device and the
>> + *		  state to which it transitioned.
>> + */
>> +void vfio_ap_on_qstate_transition(struct ap_qstate_transition *qstate_trans)
>> +{
>> +	struct vfio_ap_queue *q = vfio_ap_find_queue(qstate_trans->queue->qid);
>> +	DECLARE_BITMAP(apm_filtered, AP_DEVICES);
>> +
>> +	/*
>> +	 * If the queue is not bound to the vfio_ap device driver, then it won't
>> +	 * be passed through to a guest; so, no need to continue.
>> +	 */
>> +	if (!q)
>> +		return;
>> +
>> +	get_update_locks_for_mdev(q->matrix_mdev);
> [Severity: High]
> Can this dereference a freed pointer?
>
> If vfio_ap_find_queue() reads the device's drvdata and drops the device
> reference before returning, q has no reference count. Because
> vfio_ap_on_qstate_transition() holds no locks, a concurrent sysfs unbind
> can invoke vfio_ap_mdev_remove_queue(), which clears drvdata and calls
> kfree(q).
>
> This would cause a use-after-free when reading q->matrix_mdev.
>
> Additionally, is there a TOCTOU race when reading q->matrix_mdev without
> locks?
>
> If q->matrix_mdev is NULL here, get_update_locks_for_mdev() will skip
> acquiring kvm->lock. If another thread concurrently updates q->matrix_mdev
> to a non-NULL value while waiting on guests_lock, this could lead to issues
> later in the function.

This is a legitimate concern.
Instead of calling vfio_ap_find_queue() without locks and reading 
q->matrix_mdev,
matrix_mdev = get_update_locks_by_apqn(apqn) can be used since the apqn 
can be
obtained from qstate_trans->queue->qid. get_update_locks_by_apqn() 
acquires matrix_dev->guests_lock, searches matrix_dev->mdev_list for the 
mdev that owns the apqn, acquires matrix_mdev->kvm->lock (if attached to 
a guest), and finally acquires matrix_dev->mdevs_lock. Lookup of q via 
vfio_ap_mdev_get_queue() is done entirely under the protection of all 
three locks, completely eliminating the UAF and TOCTOU races.
>
>> +
>> +	switch (qstate_trans->new_state) {
>> +	case AP_QUEUE_CONFIG_ON:
>> +	case AP_QUEUE_CHKSTOP_OFF:
>> +		vfio_ap_mdev_reset_queue(q);
>> +		flush_work(&q->reset_work);
> [Severity: High]
> Does this also cause a use-after-free for the same reason? Passing the
> potentially freed queue object to vfio_ap_mdev_reset_queue(q) and flushing
> its work struct could trigger further memory corruption.

As stated above, using matrix_mdev = get_update_locks_by_apqn(apqn) should
also resolve this issue. If q is retrieved from matrix_mdev under
mdevs_lock, and vfio_ap_mdev_remove_queue() requires mdevs_lock to unlink
and free q, q is guaranteed valid throughout the reset and
flush_work(&q->reset_work) sequence.

>
>> +
>> +		if (q->matrix_mdev) {
>> +			if (vfio_ap_mdev_filter_matrix(q->matrix_mdev, apm_filtered)) {
>> +				vfio_ap_mdev_update_guest_apcb(q->matrix_mdev);
> [Severity: High]
> If q->matrix_mdev changed concurrently after get_update_locks_for_mdev()
> was called, could this update the KVM guest state without having acquired
> the proper KVM locks?

As stated above, using matrix_mdev = get_update_locks_by_apqn(apqn) should
also resolve this issue. If an mdev is attached to a guest
(matrix_mdev->kvm != NULL), kvm->lock is held. All matrix filtering,
APCB updates (vfio_ap_mdev_update_guest_apcb()), and queue resets operate
strictly on the locked matrix_mdev handle, with no unprotected pointer
re-reads.

>
>> +				reset_queues_for_apids(q->matrix_mdev, apm_filtered);
>> +			}
>> +		}
>> +		break;
>> +	default:
>> +		break;
>> +	}
>> +Instead of calling vfio_ap_find_queue() without locks and reading q->matrix_mdev, vfio_ap_on_qstate_transition() now calls get_update_locks_by_apqn(apqn)
>> +	release_update_locks_for_mdev(q->matrix_mdev);
> [Severity: High]
> If q->matrix_mdev was modified between the initial acquisition and here,
> would this call mutex_unlock() on a lock it did not acquire, or leave
> an acquired lock perpetually locked?

As stated above, using matrix_mdev = get_update_locks_by_apqn(apqn) should
also resolve this issue. At the exit point, the locks can be freed with
release_update_locks_for_mdev(matrix_mdev), passing the exact same
matrix_mdev that was locked by get_update_locks_by_apqn(). This guarantees
100% symmetry between lock acquisition and release under all execution
paths.

>
>> +}