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

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

[Anthony Krowiak]

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

Change log v2 => v3:
~~~~~~~~~~~~~~~~~~~
Patch 1/4: Fix leak of pinned NIB and registered NISC in \
           vfio_ap_irq_enable()
* Added fix for leak of pinned NIB and registered NISC in
  vfio_ap_irq_disable().
  ~ Changed subject line to include vfio_ap_irq_enable/disable()
* Add a bool return code from vfio_ap_wait_for_irqclear() to
  notify caller whether the IR bit in the status returned from 
  TAPQ is cleared. The caller will use this to determine whether
  the AQIC resources should be freed or not. Also:
  ~ Edited the doc block preceding the function to provide a clearer
    explanation about what the function does.
  ~ Edited the WARN_ON message to make it more understandable for someone
    that doesn't know architectural internals.
* Modified the response to the AP_RESPONSE_OTHERWISE_CHANGED case in the
  switch (status.response_code) block in vfop_ap_irq_enable(). This 
  response comes with CC=3 indicating the NIB and ISC were not accepted
  by hardware, so the AQIC resources are freed for this case.

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

 drivers/s390/crypto/vfio_ap_ops.c | 148 +++++++++++++++++++++++++-----
 1 file changed, 123 insertions(+), 25 deletions(-)

-- 
2.53.0

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

[Anthony Krowiak]

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

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

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

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

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

The fix is to unregister the NISC and an unpin the NIB in the default case
of the switch statement.

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

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

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

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

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

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

diff --git a/drivers/s390/crypto/vfio_ap_ops.c b/drivers/s390/crypto/vfio_ap_ops.c
index 940c0ff668be..64d6a8f8fa96 100644
--- a/drivers/s390/crypto/vfio_ap_ops.c
+++ b/drivers/s390/crypto/vfio_ap_ops.c
@@ -226,16 +226,24 @@ 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
+ * vfio_ap_wait_for_irqclear:
+ * Waits for the IR bit to clear thus indicating IRQs are disabled for a queue
+ *
  * @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.
+ * Repeatedly checks the IR bit for the status of a queue device by calling the
+ * PQAP(TAPQ) instruction every 20ms until: the IR bit is cleared; the response
+ * code from the PQAP instruction indicates the queue is not available or
+ * not operational; or the loop has executed more than 5 times.
+ *
+ * Return:
+ * - true if the bit is observed clear or the AP is non-operational (in which
+ *   case no further interrupts can be generated)
+ *
+ * - false if the IR bit is still set after all retries are exhausted, meaning
+ *   the hardware may still write to the NIB.
  */
-static void vfio_ap_wait_for_irqclear(int apqn)
+static bool vfio_ap_wait_for_irqclear(int apqn)
 {
 	struct ap_queue_status status;
 	int retry = 5;
@@ -246,7 +254,7 @@ static void vfio_ap_wait_for_irqclear(int apqn)
 		case AP_RESPONSE_NORMAL:
 		case AP_RESPONSE_RESET_IN_PROGRESS:
 			if (!status.irq_enabled)
-				return;
+				return true;
 			fallthrough;
 		case AP_RESPONSE_BUSY:
 			msleep(20);
@@ -257,12 +265,13 @@ static void vfio_ap_wait_for_irqclear(int apqn)
 		default:
 			WARN_ONCE(1, "%s: tapq rc %02x: %04x\n", __func__,
 				  status.response_code, apqn);
-			return;
+			return true;
 		}
 	} while (--retry);
 
-	WARN_ONCE(1, "%s: tapq rc %02x: %04x could not clear IR bit\n",
-		  __func__, status.response_code, apqn);
+	WARN_ONCE(1, "%s: tapq rc %02x: timed out verifying interrupts disabled for %02x.%04x\n",
+		  __func__, status.response_code, AP_QID_CARD(apqn), AP_QID_QUEUE(apqn));
+	return false;
 }
 
 /**
@@ -317,8 +326,21 @@ static struct ap_queue_status vfio_ap_irq_disable(struct vfio_ap_queue *q)
 		switch (status.response_code) {
 		case AP_RESPONSE_OTHERWISE_CHANGED:
 		case AP_RESPONSE_NORMAL:
-			vfio_ap_wait_for_irqclear(q->apqn);
-			goto end_free;
+			/*
+			 * AQIC disable was accepted (NORMAL), or the queue was
+			 * already disabled or a prior async request is still
+			 * completing (OTHERWISE_CHANGED).  In both cases, we must
+			 * wait until interrupt processing has been disabled
+			 * before proceeding.
+			 *
+			 * If it could not be determined whether interrupts
+			 * have been disabled, do not free the AQIC resources: the
+			 * hardware may still write to the NIB, so leave it pinned
+			 * to avoid a use-after-free. The resources will be leaked.
+			 */
+			if (vfio_ap_wait_for_irqclear(q->apqn))
+				goto end_free;
+			goto end_fail;
 		case AP_RESPONSE_RESET_IN_PROGRESS:
 		case AP_RESPONSE_BUSY:
 			msleep(20);
@@ -326,18 +348,46 @@ static struct ap_queue_status vfio_ap_irq_disable(struct vfio_ap_queue *q)
 		case AP_RESPONSE_Q_NOT_AVAIL:
 		case AP_RESPONSE_DECONFIGURED:
 		case AP_RESPONSE_CHECKSTOPPED:
+			/* AP not operational; no further interrupts possible */
+			WARN_ONCE(1, "%s: ap_aqic status %d\n", __func__,
+				  status.response_code);
+			goto end_free;
 		case AP_RESPONSE_INVALID_ADDRESS:
 		default:
-			/* All cases in default means AP not operational */
+			/*
+			 * The AQIC disable was rejected; IRQ is still enabled
+			 * and the hardware still holds the NIB address. Do not
+			 * free resources.
+			 */
 			WARN_ONCE(1, "%s: ap_aqic status %d\n", __func__,
 				  status.response_code);
-			goto end_free;
+			goto end_fail;
 		}
 	} while (retries--);
 
 	WARN_ONCE(1, "%s: ap_aqic status %d\n", __func__,
 		  status.response_code);
+
+end_fail:
+	/*
+	 * We are here either because of a failure to verify that
+	 * interrupts have been disabled, or because the AQIC instruction
+	 * failed to disable them. The AQIC resources - the pinned NIB page
+	 * and the registered guest ISC - cannot be freed here. The hardware
+	 * may still write to the NIB; freeing the pinned page would result
+	 * in a use-after-free kernel crash. The resources will therefore be
+	 * leaked. This is preferable to a use-after-free.
+	 */
+	return status;
+
 end_free:
+	/*
+	 * This label is reached because the queue was successfully disabled,
+	 * or because the queue is not operational, in which case interrupts
+	 * can not be processed, so free the AQIC resources - the pinned NIB
+	 * page and the registered guest ISC - used to enable interrupts
+	 * so they will not be leaked.
+	 */
 	vfio_ap_free_aqic_resources(q);
 	return status;
 }
@@ -495,7 +545,12 @@ static struct ap_queue_status vfio_ap_irq_enable(struct vfio_ap_queue *q,
 		q->saved_isc = isc;
 		break;
 	case AP_RESPONSE_OTHERWISE_CHANGED:
-		/* We could not modify IRQ settings: clear new configuration */
+		/*
+		 * IRQ control is already set as requested or a prior async
+		 * request has not yet completed; in either case, this response
+		 * comes with CC=3 indicating the new NIB and ISC were not accepted by
+		 * the hardware, so clean them up.
+		 */
 		ret = kvm_s390_gisc_unregister(kvm, isc);
 		if (ret)
 			VFIO_AP_DBF_WARN("%s: kvm_s390_gisc_unregister: rc=%d isc=%d, apqn=%#04x\n",
@@ -503,9 +558,12 @@ static struct ap_queue_status vfio_ap_irq_enable(struct vfio_ap_queue *q,
 		vfio_unpin_pages(&q->matrix_mdev->vdev, nib, 1);
 		break;
 	default:
-		pr_warn("%s: apqn %04x: response: %02x\n", __func__, q->apqn,
-			status.response_code);
-		vfio_ap_irq_disable(q);
+		/* We could not modify IRQ settings: clear new configuration */
+		ret = kvm_s390_gisc_unregister(kvm, isc);
+		if (ret)
+			VFIO_AP_DBF_WARN("%s: kvm_s390_gisc_unregister: rc=%d isc=%d, apqn=%#04x\n",
+					 __func__, ret, isc, q->apqn);
+		vfio_unpin_pages(&q->matrix_mdev->vdev, nib, 1);
 		break;
 	}
 
@@ -635,7 +693,6 @@ static int handle_pqap(struct kvm_vcpu *vcpu)
 	}
 
 	status = vcpu->run->s.regs.gprs[1];
-
 	/* If IR bit(16) is set we enable the interrupt */
 	if ((status >> (63 - 16)) & 0x01)
 		qstatus = vfio_ap_irq_enable(q, status & 0x07, vcpu);
-- 
2.53.0

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

[sashiko-bot]

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

New issues:
- [High] State Inconsistency / Guest Memory Corruption

Pre-existing issues:
- [High] Guest-Triggerable Host Denial of Service via WARN_ONCE
- [Medium] Potential Use-After-Free when replacing an active NIB in `vfio_ap_irq_enable`
--

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

This commit addresses resource leaks by ensuring the NIB is unpinned and the
NISC is unregistered when the PQAP(AQIC) instruction fails or times out. It
also introduces a wait loop to prevent a host use-after-free when disabling
IRQ resources.

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

[ ... ]

> @@ -257,12 +265,13 @@ static void vfio_ap_wait_for_irqclear(int apqn)
>  		default:
>  			WARN_ONCE(1, "%s: tapq rc %02x: %04x\n", __func__,
>  				  status.response_code, apqn);

[Severity: High]
This is a pre-existing issue, but could a malicious guest intentionally
trigger this warning by spamming AP queue instructions? KVM guidelines
specify that guest-reachable WARN_ONCE statements can act as a local
denial-of-service vector when panic_on_warn is enabled. Should this be
converted to pr_warn_once() or dropped entirely?

> -			return;
> +			return true;
>  		}
>  	} while (--retry);
>  
> -	WARN_ONCE(1, "%s: tapq rc %02x: %04x could not clear IR bit\n",
> -		  __func__, status.response_code, apqn);
> +	WARN_ONCE(1, "%s: tapq rc %02x: timed out verifying interrupts disabled for %02x.%04x\n",
> +		  __func__, status.response_code, AP_QID_CARD(apqn), AP_QID_QUEUE(apqn));

[Severity: High]
This is a pre-existing issue, but does this WARN_ONCE pose the same
denial-of-service risk if an untrusted guest intentionally stalls the
hardware response to exhaust the retry loop?

> +	return false;
>  }

[ ... ]

> @@ -317,8 +326,21 @@ static struct ap_queue_status vfio_ap_irq_disable(struct vfio_ap_queue *q)
>  		switch (status.response_code) {
>  		case AP_RESPONSE_OTHERWISE_CHANGED:
>  		case AP_RESPONSE_NORMAL:
> -			vfio_ap_wait_for_irqclear(q->apqn);
> -			goto end_free;
> +			/*
> +			 * AQIC disable was accepted (NORMAL), or the queue was
> +			 * already disabled or a prior async request is still
> +			 * completing (OTHERWISE_CHANGED).  In both cases, we must
> +			 * wait until interrupt processing has been disabled
> +			 * before proceeding.
> +			 *
> +			 * If it could not be determined whether interrupts
> +			 * have been disabled, do not free the AQIC resources: the
> +			 * hardware may still write to the NIB, so leave it pinned
> +			 * to avoid a use-after-free. The resources will be leaked.
> +			 */
> +			if (vfio_ap_wait_for_irqclear(q->apqn))
> +				goto end_free;
> +			goto end_fail;

[Severity: High]
If vfio_ap_wait_for_irqclear() times out, the code correctly takes the
end_fail path to prevent a host use-after-free. However, doesn't this
result in returning the original AP_RESPONSE_NORMAL status back to the
guest?

If the guest receives a success status, it might assume interrupts are
fully disabled and proceed to free and reuse the memory containing the NIB.
Since the hardware may still be actively writing to the NIB, could this
corrupt the guest's re-allocated memory? Should the driver override the
return status (e.g., returning AP_RESPONSE_BUSY) to force the guest to
retry instead?

>  		case AP_RESPONSE_RESET_IN_PROGRESS:
>  		case AP_RESPONSE_BUSY:
>  			msleep(20);
> @@ -326,18 +348,46 @@ static struct ap_queue_status vfio_ap_irq_disable(struct vfio_ap_queue *q)
>  		case AP_RESPONSE_Q_NOT_AVAIL:
>  		case AP_RESPONSE_DECONFIGURED:
>  		case AP_RESPONSE_CHECKSTOPPED:
> +			/* AP not operational; no further interrupts possible */
> +			WARN_ONCE(1, "%s: ap_aqic status %d\n", __func__,
> +				  status.response_code);

[Severity: High]
This is a pre-existing issue, but does this WARN_ONCE similarly allow an
untrusted guest to trigger a kernel panic if panic_on_warn is enabled?

> +			goto end_free;
>  		case AP_RESPONSE_INVALID_ADDRESS:
>  		default:
> -			/* All cases in default means AP not operational */
> +			/*
> +			 * The AQIC disable was rejected; IRQ is still enabled
> +			 * and the hardware still holds the NIB address. Do not
> +			 * free resources.
> +			 */
>  			WARN_ONCE(1, "%s: ap_aqic status %d\n", __func__,
>  				  status.response_code);

[Severity: High]
This is a pre-existing issue, but could this WARN_ONCE also act as a host
denial-of-service vector if a guest provides invalid queue states?

> -			goto end_free;
> +			goto end_fail;
>  		}
>  	} while (retries--);
>  
>  	WARN_ONCE(1, "%s: ap_aqic status %d\n", __func__,
>  		  status.response_code);

[Severity: High]
This is a pre-existing issue, but could an adversarial guest intentionally
induce a timeout to hit this WARN_ONCE as well?

[ ... ]

> @@ -490,6 +540,11 @@ 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 */
>  		vfio_ap_free_aqic_resources(q);
>  		q->saved_iova = nib;
>  		q->saved_isc = isc;
>  		break;

[Severity: Medium]
This is a pre-existing issue, but if ap_aqic() accepts a new NIB replacement
and returns AP_RESPONSE_NORMAL, does immediately freeing the old resources
via vfio_ap_free_aqic_resources() pose a use-after-free risk?

If the hardware processes the change asynchronously, might it still have
in-flight DMA writes directed to the old NIB (stored in q->saved_iova)? If
so, does the old NIB need to be fully quiesced using a similar wait loop
before it can be safely unpinned?

>  	case AP_RESPONSE_OTHERWISE_CHANGED:
> -		/* We could not modify IRQ settings: clear new configuration */

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

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

[Anthony Krowiak]

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

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

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

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

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

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

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

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

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

[sashiko-bot]

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

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

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

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

[Anthony Krowiak]

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

This is particularly harmful because several callers of
vfio_ap_mdev_reset_queues() and vfio_ap_mdev_reset_qlist() call
flush_work() on each 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 all
of those locks, hanging mdev removal, KVM guest teardown, and the
VFIO_DEVICE_RESET ioctl path.

Fix this by introducing AP_RESET_MAX_WAIT (2000ms) and breaking out
of the poll loop when elapsed time reaches that threshold. On
timeout the final busy status is written back to q->reset_status
so that callers inspecting reset_status.response_code after
flush_work() see a non-zero value and can return an appropriate
error.

The AQIC resources associated with this queue - the pinned page
containing the NIB and the registered guest ISC - cannot be freed
in this case. The NIB is the active DMA target for AP interrupt
delivery until the reset completes; freeing the pinned page while the
hardware may still write to it would result in a use-after-free
kernel crash. If the reset eventually completes, interrupts will be
terminated, but the pinned NIB page and ISC registration will be leaked.
This is preferable to either a use-after-free or waiting indefinitely.

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 | 27 ++++++++++++++++++++++++++-
 1 file changed, 26 insertions(+), 1 deletion(-)

diff --git a/drivers/s390/crypto/vfio_ap_ops.c b/drivers/s390/crypto/vfio_ap_ops.c
index 4c32fd6eaaa6..a9a33f4949a0 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);
@@ -2016,8 +2017,32 @@ 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) {
+			memcpy(&q->reset_status, &status, sizeof(status));
 			return;
+		}
+		if (elapsed >= AP_RESET_MAX_WAIT) {
+		/*
+		 * Timed out waiting for reset to complete.
+		 *
+		 * The AQIC resources associated with this queue - the pinned page
+		 * containing the NIB and the registered guest ISC - cannot be freed
+		 * here. The NIB is the active DMA target for AP interrupt delivery
+		 * until the reset completes; freeing the pinned page while the
+		 * hardware may still write to it would result in a use-after-free
+		 * kernel crash.
+		 *
+		 * If the reset eventually completes, interrupts will be terminated
+		 * and the pinned NIB page and ISC registration will be leaked. This
+		 * is preferable to either a use-after-free or waiting indefinitely:
+		 * the caller of apq_reset_check() holds mdevs_lock while flush_work()
+		 * blocks holds the matrix_dev->mdevs_lock mutex, which
+		 * serializes access to all mdev objects system-wide, so blocking
+		 * here would stall all other guests using AP queues.
+		 */
+			memcpy(&q->reset_status, &status, sizeof(status));
+			return;
+		}
 		if (ret == -EBUSY) {
 			pr_notice_ratelimited(WAIT_MSG, elapsed,
 					      AP_QID_CARD(q->apqn),
-- 
2.53.0

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

[sashiko-bot]

Thank you for your contribution! Sashiko AI review found 2 potential issue(s) to consider:
- [High] The timeout condition is evaluated before checking for a successful reset, causing the driver to falsely report success while skipping required resource cleanup if success occurs exactly at the timeout boundary.
- [High] The timeout path fails to clear `q->saved_isc`, leading to Cross-VM KVM interrupt routing corruption when the queue is reassigned to a new guest.
--

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

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

This commit introduces a 2000ms maximum wait time for AP queue resets in
apq_reset_check() to prevent indefinite spinning. If the reset times out,
the function breaks out of the loop and leaves the AQIC resources pinned to
avoid use-after-free issues.

> diff --git a/drivers/s390/crypto/vfio_ap_ops.c b/drivers/s390/crypto/vfio_ap_ops.c
> index 4c32fd6eaaa63..a9a33f4949a0e 100644
> --- a/drivers/s390/crypto/vfio_ap_ops.c
> +++ b/drivers/s390/crypto/vfio_ap_ops.c
[ ... ]
> @@ -2016,8 +2017,32 @@ 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) {
> +			memcpy(&q->reset_status, &status, sizeof(status));
>  			return;
> +		}
> +		if (elapsed >= AP_RESET_MAX_WAIT) {
> +		/*
> +		 * Timed out waiting for reset to complete.
> +		 *
> +		 * The AQIC resources associated with this queue - the pinned page
> +		 * containing the NIB and the registered guest ISC - cannot be freed
> +		 * here. The NIB is the active DMA target for AP interrupt delivery
> +		 * until the reset completes; freeing the pinned page while the
> +		 * hardware may still write to it would result in a use-after-free
> +		 * kernel crash.
> +		 *
> +		 * If the reset eventually completes, interrupts will be terminated
> +		 * and the pinned NIB page and ISC registration will be leaked. This
> +		 * is preferable to either a use-after-free or waiting indefinitely:
> +		 * the caller of apq_reset_check() holds mdevs_lock while flush_work()
> +		 * blocks holds the matrix_dev->mdevs_lock mutex, which
> +		 * serializes access to all mdev objects system-wide, so blocking
> +		 * here would stall all other guests using AP queues.
> +		 */
> +			memcpy(&q->reset_status, &status, sizeof(status));
> +			return;

[Severity: High]
Does this code falsely report success while skipping required resource cleanup
if the reset succeeds exactly at the timeout boundary?

Because this timeout condition is evaluated before checking if the response is
successful, if ap_tapq() returns AP_RESPONSE_NORMAL on the exact iteration
where elapsed reaches AP_RESET_MAX_WAIT (2000ms), it is treated as a timeout.

The loop exits without calling vfio_ap_free_aqic_resources(), but copies the
success status into q->reset_status, meaning the caller thinks the reset
fully succeeded.

[Severity: High]
Does this timeout path fail to clear q->saved_isc, leading to cross-vm kvm
interrupt routing corruption when the queue is reassigned to a new guest?

When apq_reset_check() times out, it skips vfio_ap_free_aqic_resources() to
leak the memory, but leaves q->saved_isc populated since the vfio_ap_queue
struct persists across mdev assignments.

When a new guest later enables AP interrupts, vfio_ap_free_aqic_resources()
uses the stale q->saved_isc to call:

    kvm_s390_gisc_unregister(new_kvm, q->saved_isc);

This unregisters the stale ISC against the new guest's KVM object,
decrementing the new guest's ISC reference count and wrongly disabling alerts
for that ISC.

> +		}
>  		if (ret == -EBUSY) {

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

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

[Anthony Krowiak]

On 8/27/26 9:43 AM, sashiko-bot@kernel.org wrote:
> Thank you for your contribution! Sashiko AI review found 2 potential issue(s) to consider:
> - [High] The timeout condition is evaluated before checking for a successful reset, causing the driver to falsely report success while skipping required resource cleanup if success occurs exactly at the timeout boundary.
> - [High] The timeout path fails to clear `q->saved_isc`, leading to Cross-VM KVM interrupt routing corruption when the queue is reassigned to a new guest.
> --
>
> commit bdd9b4236348c839f4b1d25d5fe39d6e047b2068
> Author: Anthony Krowiak <akrowiak@linux.ibm.com>
>
> s390/vfio-ap: Fix unbounded loop in apq_reset_check()
>
> This commit introduces a 2000ms maximum wait time for AP queue resets in
> apq_reset_check() to prevent indefinite spinning. If the reset times out,
> the function breaks out of the loop and leaves the AQIC resources pinned to
> avoid use-after-free issues.
>
>> diff --git a/drivers/s390/crypto/vfio_ap_ops.c b/drivers/s390/crypto/vfio_ap_ops.c
>> index 4c32fd6eaaa63..a9a33f4949a0e 100644
>> --- a/drivers/s390/crypto/vfio_ap_ops.c
>> +++ b/drivers/s390/crypto/vfio_ap_ops.c
> [ ... ]
>> @@ -2016,8 +2017,32 @@ 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) {
>> +			memcpy(&q->reset_status, &status, sizeof(status));
>>   			return;
>> +		}
>> +		if (elapsed >= AP_RESET_MAX_WAIT) {
>> +		/*
>> +		 * Timed out waiting for reset to complete.
>> +		 *
>> +		 * The AQIC resources associated with this queue - the pinned page
>> +		 * containing the NIB and the registered guest ISC - cannot be freed
>> +		 * here. The NIB is the active DMA target for AP interrupt delivery
>> +		 * until the reset completes; freeing the pinned page while the
>> +		 * hardware may still write to it would result in a use-after-free
>> +		 * kernel crash.
>> +		 *
>> +		 * If the reset eventually completes, interrupts will be terminated
>> +		 * and the pinned NIB page and ISC registration will be leaked. This
>> +		 * is preferable to either a use-after-free or waiting indefinitely:
>> +		 * the caller of apq_reset_check() holds mdevs_lock while flush_work()
>> +		 * blocks holds the matrix_dev->mdevs_lock mutex, which
>> +		 * serializes access to all mdev objects system-wide, so blocking
>> +		 * here would stall all other guests using AP queues.
>> +		 */
>> +			memcpy(&q->reset_status, &status, sizeof(status));
>> +			return;
> [Severity: High]
> Does this code falsely report success while skipping required resource cleanup
> if the reset succeeds exactly at the timeout boundary?
>
> Because this timeout condition is evaluated before checking if the response is
> successful, if ap_tapq() returns AP_RESPONSE_NORMAL on the exact iteration
> where elapsed reaches AP_RESET_MAX_WAIT (2000ms), it is treated as a timeout.
>
> The loop exits without calling vfio_ap_free_aqic_resources(), but copies the
> success status into q->reset_status, meaning the caller thinks the reset
> fully succeeded.

Ironically I was just reviewing this code and drew that same conclusion.
We need to check the return code from the call to apq_reset_check()
and if it 0, bail out of the wait loop.

Regarding copying the status into q->reset_status, that should only be
done prior to entering the loop and on ret == -EAGAIN after the
AQIC instruction is executed.

>
> [Severity: High]
> Does this timeout path fail to clear q->saved_isc, leading to cross-vm kvm
> interrupt routing corruption when the queue is reassigned to a new guest?
>
> When apq_reset_check() times out, it skips vfio_ap_free_aqic_resources() to
> leak the memory, but leaves q->saved_isc populated since the vfio_ap_queue
> struct persists across mdev assignments.
>
> When a new guest later enables AP interrupts, vfio_ap_free_aqic_resources()
> uses the stale q->saved_isc to call:
>
>      kvm_s390_gisc_unregister(new_kvm, q->saved_isc);
>
> This unregisters the stale ISC against the new guest's KVM object,
> decrementing the new guest's ISC reference count and wrongly disabling alerts
> for that ISC.

If the return code from apq_status_check() returns 0, the
AQIC resources need to be freed.

>
>> +		}
>>   		if (ret == -EBUSY) {

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

[Anthony Krowiak]

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

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

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

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

diff --git a/drivers/s390/crypto/vfio_ap_ops.c b/drivers/s390/crypto/vfio_ap_ops.c
index a9a33f4949a0..85a59730019e 100644
--- a/drivers/s390/crypto/vfio_ap_ops.c
+++ b/drivers/s390/crypto/vfio_ap_ops.c
@@ -1468,7 +1468,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);
 }
@@ -2795,11 +2795,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);
@@ -2910,7 +2910,7 @@ static void vfio_ap_mdev_cfg_add(unsigned long *apm_add, unsigned long *aqm_add,
 		bitmap_and(matrix_mdev->aqm_add,
 			   matrix_mdev->matrix.aqm, aqm_add, AP_DOMAINS);
 		bitmap_and(matrix_mdev->adm_add,
-			   matrix_mdev->matrix.adm, adm_add, AP_DEVICES);
+			   matrix_mdev->matrix.adm, adm_add, AP_DOMAINS);
 
 		mutex_unlock(&matrix_dev->mdevs_lock);
 	}
-- 
2.53.0

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

[sashiko-bot]

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

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

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