* [PATCH v9 1/6] s390/vfio-ap: Fix leaks of pinned NIB and registered GISC
[not found] <20260929121837.2715710-1-akrowiak@linux.ibm.com>
@ 2026-09-29 12:18 ` Anthony Krowiak
2026-09-29 12:18 ` [PATCH v9 2/6] s390/vfio-ap: Fix failure to release IRQ notification eventfd contexts Anthony Krowiak
` (2 subsequent siblings)
3 siblings, 0 replies; 4+ messages in thread
From: Anthony Krowiak @ 2026-09-29 12:18 UTC (permalink / raw)
To: linux-s390, linux-kernel, kvm
Cc: jjherne, borntraeger, mjrosato, pasic, alex, kwankhede, fiuczy,
pbonzini, frankja, imbrenda, agordeev, hca, gor, freude, stable
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")
The latest verion of the AP architecture limits response codes for
PQAP_ZAPQ to response codes 01, 02, 03, 04 and 0x0A; 05 is not valid. So in
apq_status_check() - which examines the response codes returned
from PQAP-TAPQ - if the response code is AP_RESPONSE_BUSY (05) - which
is valid for PQAP-TAPQ - 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()
~~~~~~~~~~~~~~~~~
When apq_reset_check() returns from the -EIO path (TAPQ returned
an invalid response code), the TAPQ status is now copied to
q->reset_status. This ensures the queue is marked not-passable —
_queue_passable() checks for AP_RESPONSE_NORMAL — so the queue is
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 which 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
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.
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 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.
After a return from vfio_ap_wait_for_irqclear(),
vfio_ap_irq_disable() frees the AQIC resources; however,
vfio_ap_wait_for_irqclear() can return for a number of reasons
which each require a different response:
* Timed out waiting for the IR bit 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 indicates the queue is disabled,
not in the host's AP configuration, or 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; so the AQIC
resources must be intentionally leaked.
The solution here is to change vfio_ap_wait_for_irqclear() to
return a code indicating the result, allowing vfio_ap_irq_disable()
to respond accordingly:
* 0: interrupt disablement is verified
* -ENODEV: the queue is not functional
* -ETIMEDOUT: timed out without verifying interrupts disabled
* -EIO: an invalid response code was returned from TAPQ
vfio_ap_free_aqic_resources()
~~~~~~~~~~~~~~~~~~~~~~~~~~~~~
The function used q->matrix_mdev->kvm and &q->matrix_mdev->vdev
when releasing the registered guest ISC and pinned NIB page
respectively. This is correct on the normal teardown path, but
becomes wrong when a queue reset times out and AQIC resources are
intentionally retained across a guest lifecycle. If the mdev is
subsequently opened by a new guest (new KVM, new IOMMU container),
a later call to vfio_ap_free_aqic_resources() — for example from
vfio_ap_irq_enable() when the new guest enables interrupts — would
release the old resources against the new guest's KVM and
vfio_device. This produces two bugs:
* kvm_s390_gisc_unregister(new_kvm, old_isc) drops a reference
the new KVM never took. If the new guest uses the same ISC
(Linux guests always use ISC 0), the refcount reaches zero and
the alert mask bit is cleared while the queue is live, silently
stopping AP interrupt delivery to the new guest.
* vfio_unpin_pages(new_vdev, old_iova) unpins from the wrong IOMMU
container. The iommufd backend hits WARN_ON(!access); the type1
backend hits WARN_ON(i != npage). The original pin in the old
container is never dropped, causing vfio to warn about a
non-empty pfn_list when that container is torn down.
Fix this by introducing struct vfio_ap_aqic_resources, which groups
the ISC, NIB IOVA, and the owning vfio_device and KVM together.
The struct replaces the standalone saved_isc and saved_iova fields
in struct vfio_ap_queue. vfio_ap_irq_enable() snapshots the owning
vdev and kvm at acquire time alongside the iova and isc.
vfio_ap_free_aqic_resources() then always releases each resource
against its original owner.
For the NIB page, aqic_resources.vdev (the vfio_device whose IOMMU
container holds the pin) is used unconditionally.
For the ISC, aqic_resources.kvm is compared against
q->matrix_mdev->kvm before calling kvm_s390_gisc_unregister(). If
they match, the same guest that registered the ISC is still running
and the unregister call proceeds normally. If they differ or the
current KVM is NULL, the original guest has already been shut down;
kvm_s390_gisa_destroy() will have zeroed the alert mask as part of
that guest's teardown, so calling unregister is unnecessary. A
warning is emitted to record that the ISC registration was not
explicitly released by this driver.
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 the
NIB page be unpinned before dma_unmap returns, or it will BUG_ON
after 10 re-notification rounds.
The fix is to fall back to a bounded queue reset via ZAPQ, which
zeroizes the NIB pointer in the hardware, eliminating the DMA risk
that justified the leak. If the reset completes, the worker frees
the AQIC resources. If the reset times out or fails, the resources
are kept leaked to guard against wild DMA writes.
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 | 433 +++++++++++++++++++++-----
drivers/s390/crypto/vfio_ap_private.h | 49 ++-
2 files changed, 390 insertions(+), 92 deletions(-)
diff --git a/drivers/s390/crypto/vfio_ap_ops.c b/drivers/s390/crypto/vfio_ap_ops.c
index 4db878c18f41..e178b657faa8 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,90 +276,231 @@ 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;
+}
+
+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 %d\n",
+ AP_QID_CARD(q->apqn), AP_QID_QUEUE(q->apqn),
+ q->aqic_resources.isc);
+ } else {
+ pr_warn_ratelimited("APQN %02x.%04x: Failed to unregister guest ISC %d\n",
+ AP_QID_CARD(q->apqn), AP_QID_QUEUE(q->apqn),
+ q->aqic_resources.isc);
+ }
}
/**
- * vfio_ap_free_aqic_resources - free vfio_ap_queue resources
+ * vfio_ap_free_aqic_resources - free vfio_ap_queue AQIC resources
* @q: The vfio_ap_queue
*
- * Unregisters the ISC in the GIB when the saved ISC not invalid.
- * Unpins the guest's page holding the NIB when it exists.
- * Resets the saved_iova and saved_isc to invalid values.
+ * Unregisters the ISC from the GIB and unpins the NIB page, using the KVM
+ * instance and vfio_device snapshotted at enable time in q->aqic_res.
+ *
+ * For the ISC: if the KVM that registered it (q->aqic_res.kvm) is still the
+ * current guest (q->matrix_mdev->kvm), call kvm_s390_gisc_unregister() to
+ * drop the registration cleanly. If the KVM has changed or is gone, the
+ * original guest's kvm_s390_gisa_destroy() will have already zeroed the alert
+ * mask on shutdown, so there is nothing to unregister and the stale pointer
+ * set to NULL so it will not be subsequently dereferenced.
+ *
+ * For the NIB: always use q->aqic_res.vdev — the vfio_device that owns the
+ * IOMMU container in which the page was pinned — regardless of what
+ * q->matrix_mdev currently points to.
*/
static void vfio_ap_free_aqic_resources(struct vfio_ap_queue *q)
{
if (!q)
return;
- if (q->saved_isc != VFIO_AP_ISC_INVALID) {
- if (!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->aqic_resources.isc != VFIO_AP_ISC_INVALID) {
+ if (q->aqic_resources.kvm &&
+ q->matrix_mdev && q->aqic_resources.kvm == q->matrix_mdev->kvm) {
+ kvm_s390_gisc_unregister(q->aqic_resources.kvm, q->aqic_resources.isc);
+ } else {
+ report_gisc_unregister_failure(q);
+ }
+ q->aqic_resources.isc = VFIO_AP_ISC_INVALID;
+ q->aqic_resources.kvm = NULL;
}
- if (q->saved_iova && !WARN_ON(!q->matrix_mdev)) {
- vfio_unpin_pages(&q->matrix_mdev->vdev, q->saved_iova, 1);
- q->saved_iova = 0;
+
+ if (q->aqic_resources.iova) {
+ if (q->aqic_resources.vdev)
+ vfio_unpin_pages(q->aqic_resources.vdev, q->aqic_resources.iova, 1);
+ else
+ pr_warn_ratelimited("APQN %02x.%04x: Failed to unpin NIB page at %pad\n",
+ AP_QID_CARD(q->apqn),
+ AP_QID_QUEUE(q->apqn),
+ &q->aqic_resources.iova);
+ q->aqic_resources.iova = 0;
+ q->aqic_resources.vdev = NULL;
}
}
/**
- * 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 +564,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 +598,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 +658,48 @@ 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;
+ q->aqic_resources.iova = nib;
+ q->aqic_resources.isc = isc;
+ q->aqic_resources.vdev = &q->matrix_mdev->vdev;
+ q->aqic_resources.kvm = q->matrix_mdev->kvm;
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->aqic_res — not the newly prepared resources.
+ * Release the newly pinned page and registered ISC;
+ * leave aqic_res 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 +825,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 +2046,29 @@ 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->aqic_resources.iova >= iova && q->aqic_resources.iova < iova + length) {
vfio_ap_irq_disable(q);
+ /*
+ * If AQIC disable was unable to confirm that interrupts were
+ * disabled (IR=0), q->aqic_res.iova remains non-zero. VFIO
+ * core requires the mapped pages to be unpinned during DMA
+ * unmap notifications (or type1 IOMMU will BUG_ON after 10
+ * retries).
+ *
+ * To satisfy VFIO core safely, fall back to a queue reset via
+ * ZAPQ. ZAPQ wipes the queue state and clears the hardware's
+ * internal NIB address register, neutralizing pending DMA.
+ *
+ * Flush the reset worker. If the reset completes, the worker
+ * frees the AQIC resources (unpinning the NIB). If the reset
+ * times out or fails with an error, the resources are kept
+ * leaked to guard against wild DMA writes.
+ */
+ if (q->aqic_resources.iova) {
+ vfio_ap_mdev_reset_queue(q);
+ flush_work(&q->reset_work);
+ }
+ }
}
}
@@ -1919,20 +2129,54 @@ 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;
case AP_RESPONSE_Q_NOT_AVAIL:
@@ -1961,10 +2205,23 @@ 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)
- return;
- if (ret == -ENODEV) {
- vfio_ap_free_aqic_resources(q);
+ 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) {
@@ -1983,8 +2240,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;
}
}
@@ -2001,7 +2262,6 @@ 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.
@@ -2014,6 +2274,15 @@ static void vfio_ap_mdev_reset_queue(struct vfio_ap_queue *q)
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),
@@ -2463,7 +2732,8 @@ int vfio_ap_mdev_probe_queue(struct ap_device *apdev)
}
q->apqn = apqn;
- q->saved_isc = VFIO_AP_ISC_INVALID;
+ q->aqic_resources.isc = VFIO_AP_ISC_INVALID;
+ /* aqic_res.iova, .vdev, .kvm all zero-initialised by kzalloc_obj */
memset(&q->reset_status, 0, sizeof(q->reset_status));
INIT_WORK(&q->reset_work, apq_reset_check);
@@ -2543,6 +2813,13 @@ void vfio_ap_mdev_remove_queue(struct ap_device *apdev)
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);
}
diff --git a/drivers/s390/crypto/vfio_ap_private.h b/drivers/s390/crypto/vfio_ap_private.h
index 9bff666b0b35..5eb0936d9183 100644
--- a/drivers/s390/crypto/vfio_ap_private.h
+++ b/drivers/s390/crypto/vfio_ap_private.h
@@ -127,29 +127,50 @@ struct ap_matrix_mdev {
DECLARE_BITMAP(adm_add, AP_DOMAINS);
};
+/**
+ * struct vfio_ap_aqic_resources - Free the AQIC resources acquired when a guest
+ * enables interrupts for an AP queue.
+ *
+ * @iova: the guest DMA address of the notification indicator byte (NIB) page,
+ * pinned via @vdev at enable time.
+ * @isc: the guest interruption sub-class registered with the GIB via @kvm at
+ * enable time.
+ * @vdev: the vfio_device whose IOMMU container owns the pin on @iova. Must be
+ * used for vfio_unpin_pages() to ensure the correct container is used,
+ * even if the queue is later re-assigned or the mdev is reopened.
+ * @kvm: the KVM instance that owns the GISC registration for @isc. Must be
+ * used for kvm_s390_gisc_unregister() to ensure the correct guest's
+ * alert mask is updated, even if the queue is later re-assigned or the
+ * mdev is reopened with a different guest.
+ */
+struct vfio_ap_aqic_resources {
+ dma_addr_t iova;
+#define VFIO_AP_ISC_INVALID 0xff
+ unsigned char isc;
+ struct vfio_device *vdev;
+ struct kvm *kvm;
+};
+
/**
* struct vfio_ap_queue - contains the data associated with a queue bound to the
* vfio_ap device driver
* @matrix_mdev: the matrix mediated device
- * @saved_iova: the notification indicator byte (nib) address
- * @apqn: the APQN of the AP queue device
- * @saved_isc: the guest ISC registered with the GIB interface
- * @mdev_qnode: allows the vfio_ap_queue struct to be added to a hashtable
+ * @aqic_res: the AQIC resources acquired when a guest enables AP interrupts
+ * @apqn: the APQN of the AP queue device
+ * @mdev_qnode: allows the vfio_ap_queue struct to be added to a hashtable
* @reset_qnode: allows the vfio_ap_queue struct to be added to a list of queues
* that need to be reset
* @reset_status: the status from the last reset of the queue
- * @reset_work: work to wait for queue reset to complete
+ * @reset_work: work to wait for queue reset to complete
*/
struct vfio_ap_queue {
- struct ap_matrix_mdev *matrix_mdev;
- dma_addr_t saved_iova;
- int apqn;
-#define VFIO_AP_ISC_INVALID 0xff
- unsigned char saved_isc;
- struct hlist_node mdev_qnode;
- struct list_head reset_qnode;
- struct ap_queue_status reset_status;
- struct work_struct reset_work;
+ struct ap_matrix_mdev *matrix_mdev;
+ struct vfio_ap_aqic_resources aqic_resources;
+ int apqn;
+ struct hlist_node mdev_qnode;
+ struct list_head reset_qnode;
+ struct ap_queue_status reset_status;
+ struct work_struct reset_work;
};
int vfio_ap_mdev_register(void);
--
2.53.0
^ permalink raw reply related [flat|nested] 4+ messages in thread* [PATCH v9 3/6] s390/vfio-ap: Fix unbounded loop in apq_reset_check()
[not found] <20260929121837.2715710-1-akrowiak@linux.ibm.com>
2026-09-29 12:18 ` [PATCH v9 1/6] s390/vfio-ap: Fix leaks of pinned NIB and registered GISC Anthony Krowiak
2026-09-29 12:18 ` [PATCH v9 2/6] s390/vfio-ap: Fix failure to release IRQ notification eventfd contexts Anthony Krowiak
@ 2026-09-29 12:18 ` Anthony Krowiak
2026-09-29 12:18 ` [PATCH v9 5/6] s390/vfio-ap: fix queue state leakage to guest and host Anthony Krowiak
3 siblings, 0 replies; 4+ messages in thread
From: Anthony Krowiak @ 2026-09-29 12:18 UTC (permalink / raw)
To: linux-s390, linux-kernel, kvm
Cc: jjherne, borntraeger, mjrosato, pasic, alex, kwankhede, fiuczy,
pbonzini, frankja, imbrenda, agordeev, hca, gor, freude, stable
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_STATE_CHANGE_IN_PROGRESS, 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 | 122 +++++++++++++++++++++++++++---
1 file changed, 111 insertions(+), 11 deletions(-)
diff --git a/drivers/s390/crypto/vfio_ap_ops.c b/drivers/s390/crypto/vfio_ap_ops.c
index 087e8474a34a..07fbfa6f1015 100644
--- a/drivers/s390/crypto/vfio_ap_ops.c
+++ b/drivers/s390/crypto/vfio_ap_ops.c
@@ -2164,6 +2164,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:
/*
@@ -2190,8 +2196,59 @@ static int apq_status_check(int apqn, struct ap_queue_status *status)
}
}
+static void report_apq_reset_check_timeout(struct vfio_ap_queue *q)
+{
+ if (q->aqic_resources.isc != VFIO_AP_ISC_INVALID || q->aqic_resources.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;
@@ -2223,30 +2280,73 @@ static void apq_reset_check(struct work_struct *reset_work)
*/
memcpy(&q->reset_status, &status, sizeof(status));
return;
- }
- if (ret == -EBUSY) {
+ } else if (elapsed >= AP_RESET_MAX_WAIT) {
+ /*Timed out without being able to verify zapq completed */
+ 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;
+ }
+
+ report_apq_reset_check_timeout(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;
+ } else if (ret == -EBUSY) {
pr_notice_ratelimited(WAIT_MSG, elapsed,
AP_QID_CARD(q->apqn),
AP_QID_QUEUE(q->apqn),
status.response_code,
status.queue_empty,
status.irq_enabled);
+ continue;
} 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) {
+ 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));
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.
+ * We are here for one of two reasons:
+ *
+ * Zeroization confirmed (ret == 0): the TAPQ status bits
+ * indicate the async portion of the ZAPQ completed
+ * successfully.
+ *
+ * Queue non-operational (ret == -ENODEV): the queue is
+ * deconfigured or checkstopped; interrupts are not
+ * possible. Zeroization cannot be confirmed in this state,
+ * but the queue cannot generate interrupts, so the
+ * NIB page is no longer a DMA.
+ *
+ * In either case, it is safe to free up the AQIC
+ * resources.
*/
- vfio_ap_free_aqic_resources(q);
- break;
+ apq_reset_finalize(q, &status, ret);
+ return;
}
}
}
--
2.53.0
^ permalink raw reply related [flat|nested] 4+ messages in thread* [PATCH v9 5/6] s390/vfio-ap: fix queue state leakage to guest and host
[not found] <20260929121837.2715710-1-akrowiak@linux.ibm.com>
` (2 preceding siblings ...)
2026-09-29 12:18 ` [PATCH v9 3/6] s390/vfio-ap: Fix unbounded loop in apq_reset_check() Anthony Krowiak
@ 2026-09-29 12:18 ` Anthony Krowiak
3 siblings, 0 replies; 4+ messages in thread
From: Anthony Krowiak @ 2026-09-29 12:18 UTC (permalink / raw)
To: linux-s390, linux-kernel, kvm
Cc: jjherne, borntraeger, mjrosato, pasic, alex, kwankhede, fiuczy,
pbonzini, frankja, imbrenda, agordeev, hca, gor, freude, stable
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 security requirements and prevent resource
leakage and corruption in the hypervisor.
That is a legitimate concern; however, functions initiating
the reset all hold matrix_dev->mdevs_lock, which guards access
to all mdevs under the control of the vfio_ap device driver.
Blocking that lock prevents a system administrator from
configuring mdevs (assigning/unassigning adapters, domains,
and control domains via the mdev sysfs interfaces) and may
hang any guest started using one of those 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 not confirmed zeroized must not be passed
through to a guest, as it may contain key material from a
previous guest or host operation.
_queue_passable() is therefore changed to return true only
when reset_status.response_code == AP_RESPONSE_NORMAL. A new
helper, apq_reset_finalize(), is introduced to ensure that
q->reset_status correctly reflects the confirmed end state.
It copies the full TAPQ status word into q->reset_status and
overrides the response code with 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 preserved so
_queue_passable() correctly returns false.
Since _queue_passable() now rejects deconfigured and
checkstopped queues, a queue bound to the vfio_ap driver
cannot be passed through to a guest even after it returns
to an operational state. To resolve this, a new callback,
on_qstate_transition, is added to struct ap_driver and
invoked by the AP bus scan when a queue transitions between
operational and non-operational states. The vfio_ap driver
implements this callback in vfio_ap_on_qstate_transition(),
which dispatches to two helpers:
vfio_ap_on_queue_operational() handles CONFIG_ON and
CHKSTOP_OFF. If the queue is already live in a guest's
shadow APCB, the guest owns it and is responsible for
handling the transition. Otherwise the queue is reset to
clear the stale non-zero response code left by the prior
failed reset, so that filter_matrix will correctly re-admit
it. If the queue is assigned to an mdev, filter_matrix is
called to update the shadow APCB; if the shadow changed and
the mdev is attached to a running guest, the guest APCB is
updated so the queue becomes available to the guest. Any
other adapters newly filtered out are also reset.
vfio_ap_on_queue_non_operational() handles CONFIG_OFF and
CHKSTOP_ON. If the queue is already live in a guest's shadow
APCB, the guest owns it and is responsible for handling the
transition. Otherwise, q->reset_status is zeroed and its
response code is set to AP_RESPONSE_DECONFIGURED or
AP_RESPONSE_CHECKSTOPPED as appropriate, ensuring
_queue_passable() returns false and filter_matrix will not
admit the non-operational queue to a guest. If the queue is
assigned to an mdev, filter_matrix is called to remove it
from the shadow APCB; if the shadow changed and the mdev is
attached to a running guest, the guest APCB is updated. Any
adapters filtered out are reset.
Note: reset_queues_for_apids() will issue a ZAPQ for the
non-operational queue itself (since its APID was filtered).
That ZAPQ will fail with DECONFIGURED or CHECKSTOPPED, which
apq_reset_finalize() handles gracefully. Avoiding this extra
ZAPQ would require modifying reset_queues_for_apids() or
collect_queues_to_reset() to skip queues with a non-zero
reset_status.response_code, adding complexity not justified
by the benefit on this infrequent path.
Additionally, vfio_ap_mdev_probe_queue() calls
vfio_ap_mdev_reset_queue() and flush_work() while holding
the update locks to guarantee a clean queue before it can
be assigned to a guest. This adds at least AP_RESET_INTERVAL
(20 ms) of lock-hold time per probed queue, since
apq_reset_check() sleeps before its first TAPQ. In theory
this is O(N) serialized under the global mdevs_lock; in
practice, AP hardware constraints and typical deployment
sizes bound the number of queues allocated for guest use in
a single LPAR to a modest number, so the total probe cost
is not necessarily a real-world concern.
Deferring the reset outside the lock-held section is not
safe: apq_reset_check() accesses q->reset_status and
q->aqic_resources without holding any lock, and
vfio_ap_mdev_remove_queue() can run concurrently and call
kfree(q) after releasing the locks, leaving the worker with
a dangling pointer. flush_work() inside the lock-held
section serializes against remove_queue and prevents this.
The fundamental remedy would be to replace the global
mdevs_lock with a per-mdev lock, scoping the blocking to
the single mdev whose queue is being probed and allowing
resets across different mdevs to proceed in parallel. That
is a non-trivial redesign of the locking architecture and
is deferred to a separate effort.
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 | 33 ++++++++
drivers/s390/crypto/vfio_ap_drv.c | 1 +
drivers/s390/crypto/vfio_ap_ops.c | 115 ++++++++++++++++++++++++--
drivers/s390/crypto/vfio_ap_private.h | 2 +
5 files changed, 193 insertions(+), 9 deletions(-)
diff --git a/drivers/s390/crypto/ap_bus.c b/drivers/s390/crypto/ap_bus.c
index d82df5b4e2db..27b67bd04646 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_on_qstate_transition */
+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..bbbd9aa4820e 100644
--- a/drivers/s390/crypto/ap_bus.h
+++ b/drivers/s390/crypto/ap_bus.h
@@ -132,6 +132,33 @@ struct ap_message;
*/
#define AP_DRIVER_FLAG_DEFAULT 0x0001
+/**
+ * ap_queue_transition_state - enumerates the new states to which a
+ * queue can transition.
+ * @AP_QUEUE_CONFIG_ON: from deconfigured to configured
+ * @AP_QUEUE_CONFIG_OFF: from configured to deconfigured
+ * @AP_QUEUE_CHKSTOP_ON: from not checkstopped to checkstopped
+ * @AP_QUEUE_CHKSTOP_OFF: from checkstopped to not checkstopped
+ */
+enum ap_queue_transition_state {
+ AP_QUEUE_CONFIG_ON,
+ AP_QUEUE_CONFIG_OFF,
+ AP_QUEUE_CHKSTOP_ON,
+ AP_QUEUE_CHKSTOP_OFF,
+};
+
+/**
+ * struct ap_qstate_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
+ */
+struct ap_qstate_transition {
+ struct ap_queue *queue;
+ enum ap_queue_transition_state new_state;
+};
+
struct ap_driver {
struct device_driver driver;
@@ -155,6 +182,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 ad19d44a67bc..89efb73d7035 100644
--- a/drivers/s390/crypto/vfio_ap_ops.c
+++ b/drivers/s390/crypto/vfio_ap_ops.c
@@ -881,14 +881,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;
}
/*
@@ -2855,6 +2855,8 @@ int vfio_ap_mdev_probe_queue(struct ap_device *apdev)
if (matrix_mdev) {
vfio_ap_mdev_link_queue(matrix_mdev, q);
+ vfio_ap_mdev_reset_queue(q);
+ flush_work(&q->reset_work);
/*
* If we're in the process of handling the adding of adapters or
@@ -2944,8 +2946,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);
}
/**
@@ -3351,3 +3353,98 @@ 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)
+{
+ DECLARE_BITMAP(apm_filtered, AP_DEVICES);
+ int apqn = qstate_trans->queue->qid;
+ struct ap_matrix_mdev *matrix_mdev;
+ struct vfio_ap_queue *q;
+ /*
+ * Take all three locks in order and look up the queue under them.
+ * This is the same pattern used by vfio_ap_mdev_probe_queue() and
+ * avoids two races present if vfio_ap_find_queue() is called first:
+ *
+ * 1. vfio_ap_mdev_remove_queue() can kfree(q) between the find and
+ * the lock acquisition, producing a use-after-free on q->matrix_mdev.
+ *
+ * 2. q->matrix_mdev can change (assign/unassign under guests_lock +
+ * mdevs_lock) between the find and get_update_locks_for_mdev(),
+ * causing kvm->lock to be taken without being released, or released
+ * without having been taken.
+ */
+ matrix_mdev = get_update_locks_by_apqn(apqn);
+ if (!matrix_mdev)
+ goto out_unlock;
+
+ q = vfio_ap_mdev_get_queue(matrix_mdev, apqn);
+ if (!q)
+ goto out_unlock;
+
+ switch (qstate_trans->new_state) {
+ case AP_QUEUE_CONFIG_ON:
+ case AP_QUEUE_CHKSTOP_OFF:
+ /*
+ * The queue has returned to operational state. Reset it to a
+ * known-clean state before re-admitting it to the guest's APCB.
+ * A queue returning from deconfigured or checkstopped state may
+ * have stale association state, undelivered replies, or a residual
+ * NIB address register; ZAPQ returns it to a known-clean baseline.
+ * The guest AP bus handles re-initialisation (including
+ * re-enabling interrupts) after a queue reappears; the brief stall
+ * on any pending guest AQIC is an expected consequence of the
+ * hardware event, not a driver bug.
+ *
+ * matrix_mdev is guaranteed non-NULL here: get_update_locks_by_apqn()
+ * returns NULL (and we branch to out_unlock) when the queue is not
+ * assigned to any mdev, so the check is unnecessary.
+ */
+ vfio_ap_mdev_reset_queue(q);
+ flush_work(&q->reset_work);
+
+ if (vfio_ap_mdev_filter_matrix(matrix_mdev, apm_filtered)) {
+ vfio_ap_mdev_update_guest_apcb(matrix_mdev);
+ reset_queues_for_apids(matrix_mdev, apm_filtered);
+ }
+ break;
+ case AP_QUEUE_CONFIG_OFF:
+ case AP_QUEUE_CHKSTOP_ON:
+ /*
+ * The queue has become non-operational. Hot-unplug its adapter
+ * from the guest's shadow APCB so the guest stops issuing
+ * operations to a queue it can no longer reach. Since queues are
+ * addressed via a card/domain matrix it is not possible to remove
+ * a single queue; the whole adapter must be unplugged.
+ *
+ * The queue remains bound to the driver and stays in
+ * matrix_dev->info; vfio_ap_mdev_filter_matrix() will
+ * re-admit it automatically when the queue returns to an
+ * operational state (CONFIG_ON or CHKSTOP_OFF).
+ */
+ if (test_bit_inv(AP_QID_CARD(q->apqn),
+ matrix_mdev->shadow_apcb.apm) &&
+ test_bit_inv(AP_QID_QUEUE(q->apqn),
+ matrix_mdev->shadow_apcb.aqm)) {
+ clear_bit_inv(AP_QID_CARD(q->apqn),
+ matrix_mdev->shadow_apcb.apm);
+ vfio_ap_mdev_update_guest_apcb(matrix_mdev);
+ reset_queues_for_apid(matrix_mdev,
+ AP_QID_CARD(q->apqn));
+ }
+ break;
+ default:
+ break;
+ }
+
+out_unlock:
+ release_update_locks_for_mdev(matrix_mdev);
+}
diff --git a/drivers/s390/crypto/vfio_ap_private.h b/drivers/s390/crypto/vfio_ap_private.h
index 5eb0936d9183..6abf2cc1fd86 100644
--- a/drivers/s390/crypto/vfio_ap_private.h
+++ b/drivers/s390/crypto/vfio_ap_private.h
@@ -186,4 +186,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
^ permalink raw reply related [flat|nested] 4+ messages in thread