[PATCH 0/4] Fix pre-existing bugs in vfio_ap device driver

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

This series is based on the 'next' branch in the 
https://git.kernel.org/pub/scm/linux/kernel/git/kvms390/linux.git
source tree.

Anthony Krowiak (4):
  s390/vfio-ap: Fix leak of pinned NIB and registered NISC in
    vfio_ap_irq_enable()
  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 | 36 +++++++++++++++++++++++++++----
 1 file changed, 32 insertions(+), 4 deletions(-)

-- 
2.53.0

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

[Anthony Krowiak]

The vfio_ap_irq_enable() function executes the PQAP(AQIC) instruction to
enable 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. 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.

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 | 11 ++++++++---
 1 file changed, 8 insertions(+), 3 deletions(-)

diff --git a/drivers/s390/crypto/vfio_ap_ops.c b/drivers/s390/crypto/vfio_ap_ops.c
index 940c0ff668be..a46bf381ab72 100644
--- a/drivers/s390/crypto/vfio_ap_ops.c
+++ b/drivers/s390/crypto/vfio_ap_ops.c
@@ -503,9 +503,14 @@ 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);
+		pr_warn("%s: PQAP(AQIC) failed with response code %02x for apqn %04x\n",
+			__func__, status.response_code, q->apqn);
+		/* 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;
 	}
 
-- 
2.53.0

[PATCH 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>
---
 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 a46bf381ab72..6e4569d6b975 100644
--- a/drivers/s390/crypto/vfio_ap_ops.c
+++ b/drivers/s390/crypto/vfio_ap_ops.c
@@ -2066,12 +2066,28 @@ static int vfio_ap_mdev_open_device(struct vfio_device *vdev)
 	return vfio_ap_mdev_set_kvm(matrix_mdev, vdev->kvm);
 }
 
+static void vfio_ap_mdev_release_eventfds(struct ap_matrix_mdev *matrix_mdev)
+{
+	if (matrix_mdev->req_trigger) {
+		eventfd_ctx_put(matrix_mdev->req_trigger);
+		matrix_mdev->req_trigger = NULL;
+	}
+	if (matrix_mdev->cfg_chg_trigger) {
+		eventfd_ctx_put(matrix_mdev->cfg_chg_trigger);
+		matrix_mdev->cfg_chg_trigger = NULL;
+	}
+}
+
 static void vfio_ap_mdev_close_device(struct vfio_device *vdev)
 {
 	struct ap_matrix_mdev *matrix_mdev =
 		container_of(vdev, struct ap_matrix_mdev, vdev);
 
 	vfio_ap_mdev_unset_kvm(matrix_mdev);
+
+	mutex_lock(&matrix_dev->mdevs_lock);
+	vfio_ap_mdev_release_eventfds(matrix_mdev);
+	mutex_unlock(&matrix_dev->mdevs_lock);
 }
 
 static void vfio_ap_mdev_request(struct vfio_device *vdev, unsigned int count)
-- 
2.53.0

[PATCH 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_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_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_TIMEOUT (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. vfio_ap_free_aqic_resources() is called before returning
to release any KVM ISC registration and pinned NIB page,
consistent with all other early-exit paths in the function.

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

diff --git a/drivers/s390/crypto/vfio_ap_ops.c b/drivers/s390/crypto/vfio_ap_ops.c
index 6e4569d6b975..c7eebbd0ed40 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);
@@ -1973,6 +1974,12 @@ static void apq_reset_check(struct work_struct *reset_work)
 					      status.response_code,
 					      status.queue_empty,
 					      status.irq_enabled);
+			if (elapsed >= AP_RESET_MAX_WAIT) {
+				/* Timed out waiting for reset to complete */
+				memcpy(&q->reset_status, &status, sizeof(status));
+				vfio_ap_free_aqic_resources(q);
+				return;
+			}
 		} else {
 			if (q->reset_status.response_code == AP_RESPONSE_RESET_IN_PROGRESS ||
 			    q->reset_status.response_code == AP_RESPONSE_BUSY ||
-- 
2.53.0

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

[Anthony Krowiak]

In vfio_ap_mdev_cfg_add(), the bitmap_and() call that computes
the intersection of newly added control domains with those
assigned to each matrix mdev passes AP_DEVICES as the bitmap
size instead of AP_DOMAINS:

    bitmap_and(matrix_mdev->adm_add,
               matrix_mdev->matrix.adm, adm_add, AP_DEVICES);

Control domains are indexed by AP queue index (APQI) and their
bitmaps are sized by AP_DOMAINS, not AP_DEVICES. The two
constants are both 256 today so there is no functional impact,
but the wrong constant is inconsistent with the two bitmap_and()
calls immediately above it and with every other operation on
adm/aqm bitmaps in the file, all of which correctly use
AP_DOMAINS.

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

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

diff --git a/drivers/s390/crypto/vfio_ap_ops.c b/drivers/s390/crypto/vfio_ap_ops.c
index c7eebbd0ed40..e62e0dfa7282 100644
--- a/drivers/s390/crypto/vfio_ap_ops.c
+++ b/drivers/s390/crypto/vfio_ap_ops.c
@@ -2840,7 +2840,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 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/20260824135850.503728-1-akrowiak@linux.ibm.com?part=2

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

[sashiko-bot]

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

Pre-existing issues:
- [High] Incomplete Fix / Latent Stack Buffer Overflow due to Inconsistent Constant Usage
--

--- Patch [4]: [PATCH 4/4] s390/vfio-ap: Use AP_DOMAINS for adm_add bitmap size in vfio_ap_mdev_cfg_add() ---
commit 35cf2442d556c90b8b97a43b8f409fe51d21bae0
Author: Anthony Krowiak <akrowiak@linux.ibm.com>

s390/vfio-ap: Use AP_DOMAINS for adm_add bitmap size in vfio_ap_mdev_cfg_add()

This commit changes the bitmap size constant from AP_DEVICES to AP_DOMAINS
in vfio_ap_mdev_cfg_add() for the adm_add bitmap. The change prevents future
issues if the constants' values ever diverge.

>     Control domains are indexed by AP queue index (APQI) and their
>     bitmaps are sized by AP_DOMAINS, not AP_DEVICES. The two
>     constants are both 256 today so there is no functional impact,
>     but the wrong constant is inconsistent with the two bitmap_and()
>     calls immediately above it and with every other operation on
>     adm/aqm bitmaps in the file, all of which correctly use
>     AP_DOMAINS.

[Severity: High]
This is a pre-existing issue, but does every other operation correctly use
AP_DOMAINS?

Looking at vfio_ap_mdev_on_cfg_remove(), it appears AP_DEVICES is still used
for domain masks sized by AP_DOMAINS:

drivers/s390/crypto/vfio_ap_ops.c:vfio_ap_mdev_on_cfg_remove() {
...
	do_remove |= bitmap_andnot(aqrem,
				   (unsigned long *)prev_config_info->aqm,
				   (unsigned long *)cur_config_info->aqm,
				   AP_DEVICES);
	do_remove |= bitmap_andnot(cdrem,
				   (unsigned long *)prev_config_info->adm,
				   (unsigned long *)cur_config_info->adm,
				   AP_DEVICES);
...
}

Similarly, in vfio_ap_mdev_hot_unplug_domain():

drivers/s390/crypto/vfio_ap_ops.c:vfio_ap_mdev_hot_unplug_domain() {
...
	DECLARE_BITMAP(apqis, AP_DOMAINS);

	bitmap_zero(apqis, AP_DEVICES);
...
}

If the constants AP_DEVICES and AP_DOMAINS ever diverge, could these
operations cause out-of-bounds stack memory accesses?

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

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

[sashiko-bot]

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

Pre-existing issues:
- [Critical] `vfio_ap_irq_enable` incorrectly treats `AP_RESPONSE_OTHERWISE_CHANGED` (0x07) as a failure, unpinning the NIB while the hardware has successfully configured it. This allows the AP hardware to DMA into freed host memory.
- [High] Unprivileged guest can flood the host dmesg by repeatedly triggering a `pr_warn()` in the `default` case of the `ap_aqic()` response code switch.
- [High] A malicious or misbehaving guest can trigger host-side WARN_ONCE macros by disabling IRQs on a checkstopped or busy AP queue, leading to kernel panics if panic_on_warn is enabled.
--

--- Patch [1]: [PATCH 1/4] s390/vfio-ap: Fix leak of pinned NIB and registered NISC in vfio_ap_irq_enable() ---
commit c278a02f835924e412b00c7f6b715a44741cbcce
Author: Anthony Krowiak <akrowiak@linux.ibm.com>

s390/vfio-ap: Fix leak of pinned NIB and registered NISC in vfio_ap_irq_enable()

This commit addresses a resource leak of pinned NIB and registered NISC
pages during AP queue interrupt enablement. It updates the error handling
path to unpin and unregister resources directly when an unexpected status
is returned. This avoids incorrectly invoking the interrupt disable routine
on a failing queue.

> diff --git a/drivers/s390/crypto/vfio_ap_ops.c b/drivers/s390/crypto/vfio_ap_ops.c
> index 940c0ff668bee..a46bf381ab726 100644
> --- a/drivers/s390/crypto/vfio_ap_ops.c
> +++ b/drivers/s390/crypto/vfio_ap_ops.c
> @@ -503,9 +503,14 @@ static struct ap_queue_status vfio_ap_irq_enable(struct vfio_ap_queue *q,
>  		vfio_unpin_pages(&q->matrix_mdev->vdev, nib, 1);
>  		break;

[Severity: Critical]
This is a pre-existing issue, but does vfio_ap_irq_enable() incorrectly
treat AP_RESPONSE_OTHERWISE_CHANGED (0x07) as a failure?

When a guest triggers PQAP(AQIC) to enable interrupts on an AP queue that
is not empty, the hardware returns AP_RESPONSE_OTHERWISE_CHANGED. This
indicates successful IRQ modification but a non-empty queue.

Because this code treats it as an error and unpins the NIB pages, couldn't
the AP hardware later DMA interrupt data into the physical address of the
unpinned NIB, corrupting host memory if those pages are reallocated?

>  	default:
> -		pr_warn("%s: apqn %04x: response: %02x\n", __func__, q->apqn,
> -			status.response_code);

[Severity: High]
This isn't a bug introduced by this patch, but is it safe to leave this
pr_warn() un-ratelimited in the default case?

If an unprivileged KVM guest executes PQAP(AQIC) in a tight loop on a
queue that is in a state returning an unhandled response code (e.g.,
AP_RESPONSE_Q_NOT_AVAIL), couldn't they flood the host dmesg?

This violates KVM subsystem rules which mandate that guest-reachable
warnings must be rate-limited or removed to prevent log flood Denial of
Service.

> -		vfio_ap_irq_disable(q);

[Severity: High]
This patch correctly removes the call to vfio_ap_irq_disable() here, but
this highlights a pre-existing issue: is it possible for a malicious guest
to trigger the WARN_ONCE macros inside vfio_ap_irq_disable() directly?

If the guest executes PQAP(AQIC) with the interrupt enable bit set to 0
(disable) on a queue that returns AP_RESPONSE_DECONFIGURED or
AP_RESPONSE_CHECKSTOPPED, it will directly invoke vfio_ap_irq_disable():

drivers/s390/crypto/vfio_ap_ops.c:vfio_ap_irq_disable() {
    [ ... ]
	case AP_RESPONSE_Q_NOT_AVAIL:
	case AP_RESPONSE_DECONFIGURED:
	case AP_RESPONSE_CHECKSTOPPED:
	case AP_RESPONSE_INVALID_ADDRESS:
	default:
		/* All cases in default means AP not operational */
		WARN_ONCE(1, "%s: ap_aqic status %d\n", __func__,
			  status.response_code);
    [ ... ]
}

Since this path is fully reachable by an unprivileged guest, couldn't this
lead to a host kernel panic if panic_on_warn is enabled?

> +		pr_warn("%s: PQAP(AQIC) failed with response code %02x for apqn %04x\n",
> +			__func__, status.response_code, q->apqn);
> +		/* 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;
>  	}

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

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

[sashiko-bot]

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

New issues:
- [Critical] Unpinning the guest NIB page via `vfio_ap_free_aqic_resources()` when `apq_reset_check()` times out causes a critical DMA use-after-free vulnerability.

Pre-existing issues:
- [Critical] Unpinning the guest NIB page after `ap_aqic` timeout in `vfio_ap_irq_disable()` causes a DMA use-after-free.
- [High] The introduced timeout mechanism fails to break out of the infinite loop when the hardware returns an asynchronous error (yielding `-EAGAIN`).
--

--- Patch [3]: [PATCH 3/4] s390/vfio-ap: Fix unbounded loop in apq_reset_check() ---
commit afa7b19f4ae6cc32897da341d31f38a06cb912ec
Author: Anthony Krowiak <akrowiak@linux.ibm.com>

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

This commit introduces a timeout mechanism (AP_RESET_MAX_WAIT) in
apq_reset_check() to prevent the worker from spinning indefinitely when
waiting for an AP queue reset. If the reset takes longer than 2000ms, the
worker writes back the status, frees AQIC resources, and exits.

> diff --git a/drivers/s390/crypto/vfio_ap_ops.c b/drivers/s390/crypto/vfio_ap_ops.c
> index 6e4569d6b9753..c7eebbd0ed40e 100644
> --- a/drivers/s390/crypto/vfio_ap_ops.c
> +++ b/drivers/s390/crypto/vfio_ap_ops.c
[ ... ]
> @@ -1973,6 +1974,12 @@ static void apq_reset_check(struct work_struct *reset_work)
>  					      status.response_code,
>  					      status.queue_empty,
>  					      status.irq_enabled);
> +			if (elapsed >= AP_RESET_MAX_WAIT) {
> +				/* Timed out waiting for reset to complete */
> +				memcpy(&q->reset_status, &status, sizeof(status));
> +				vfio_ap_free_aqic_resources(q);

[Severity: Critical]
If the hardware gets stuck and we time out here, does unpinning the NIB page
cause a DMA use-after-free regression?

If the hardware is stuck in a busy or reset-in-progress state, it might still
hold the physical address of the NIB.

vfio_ap_free_aqic_resources() unpins the active DMA page:

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

If the hardware later completes an operation or sends an interrupt, could it
execute a DMA write to the now-freed physical page, potentially corrupting
arbitrary memory on the host?

To safely handle permanently busy hardware, does the NIB page need to be
intentionally leaked or quarantined rather than unpinned?

> +				return;
> +			}
>  		} else {
>  			if (q->reset_status.response_code == AP_RESPONSE_RESET_IN_PROGRESS ||
>  			    q->reset_status.response_code == AP_RESPONSE_BUSY ||
>  			    q->reset_status.response_code == AP_RESPONSE_STATE_CHANGE_IN_PROGRESS ||
>  			    ret == -EAGAIN) {
>  				status = ap_zapq(q->apqn, 0);
>  				memcpy(&q->reset_status, &status, sizeof(status));
>  				continue;

[Severity: High]
This isn't a bug introduced by this patch, but does this logic still allow an
unbounded loop regression?

Because the elapsed timeout check is entirely inside the if (ret == -EBUSY)
block, if apq_status_check() repeatedly returns -EAGAIN, we take this else
branch and continue without ever checking the elapsed time.

Could this still result in the worker thread hanging indefinitely and
permanently blocking global locks?

>  			}

[Severity: Critical]
This is a pre-existing issue, but a similar DMA use-after-free regression
seems to exist in vfio_ap_irq_disable():

vfio_ap_irq_disable() {
    ...
end_free:
    vfio_ap_free_aqic_resources(q);
    return status;
}

If the hardware fails to process the ap_aqic disable command after its retries,
this code unconditionally calls vfio_ap_free_aqic_resources(), unpinning the
NIB page.

If the hardware is still actively using it and an interrupt fires later,
couldn't this also cause a DMA write to a freed page?

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

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

[Jason J. Herne]

On 8/24/26 9:58 AM, Anthony Krowiak wrote:
> In vfio_ap_mdev_cfg_add(), the bitmap_and() call that computes
> the intersection of newly added control domains with those
> assigned to each matrix mdev passes AP_DEVICES as the bitmap
> size instead of AP_DOMAINS:
> 
>      bitmap_and(matrix_mdev->adm_add,
>                 matrix_mdev->matrix.adm, adm_add, AP_DEVICES);
> 
> Control domains are indexed by AP queue index (APQI) and their
> bitmaps are sized by AP_DOMAINS, not AP_DEVICES. The two
> constants are both 256 today so there is no functional impact,
> but the wrong constant is inconsistent with the two bitmap_and()
> calls immediately above it and with every other operation on
> adm/aqm bitmaps in the file, all of which correctly use
> AP_DOMAINS.
> 
> Use AP_DOMAINS to keep the code consistent and correct in case
> the two constants ever diverge.
> 
> Signed-off-by: Anthony Krowiak <akrowiak@linux.ibm.com>
> ---
>   drivers/s390/crypto/vfio_ap_ops.c | 2 +-
>   1 file changed, 1 insertion(+), 1 deletion(-)
> 
> diff --git a/drivers/s390/crypto/vfio_ap_ops.c b/drivers/s390/crypto/vfio_ap_ops.c
> index c7eebbd0ed40..e62e0dfa7282 100644
> --- a/drivers/s390/crypto/vfio_ap_ops.c
> +++ b/drivers/s390/crypto/vfio_ap_ops.c
> @@ -2840,7 +2840,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);
>   	}

Reviewed-by: Jason J. Herne <jjherne@linux.ibm.com>

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

[Matthew Rosato]

On 8/24/26 9:58 AM, Anthony Krowiak wrote:
> The vfio_ap_irq_enable() function executes the PQAP(AQIC) instruction to
> enable 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. 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.
> 
> 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 | 11 ++++++++---
>  1 file changed, 8 insertions(+), 3 deletions(-)
> 
> diff --git a/drivers/s390/crypto/vfio_ap_ops.c b/drivers/s390/crypto/vfio_ap_ops.c
> index 940c0ff668be..a46bf381ab72 100644
> --- a/drivers/s390/crypto/vfio_ap_ops.c
> +++ b/drivers/s390/crypto/vfio_ap_ops.c
> @@ -503,9 +503,14 @@ 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);
> +		pr_warn("%s: PQAP(AQIC) failed with response code %02x for apqn %04x\n",
> +			__func__, status.response_code, q->apqn);

LGTM, except Sashiko mentions the pr_warn here that you are updating has
a pre-existing issue.

Since you're touching it already, do you think it makes sense to switch
to pr_warn_ratelimited with this patch?

> +		/* 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;
>  	}
>  

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

[Matthew Rosato]

On 8/24/26 9:58 AM, Anthony Krowiak wrote:
> 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>

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

[Matthew Rosato]

On 8/24/26 9:58 AM, Anthony Krowiak wrote:
> 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_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_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_TIMEOUT (2000ms) and breaking out

s/AP_RESET_TIMEOUT/AP_RESET_MAX_WAIT/ ?

> 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. vfio_ap_free_aqic_resources() is called before returning
> to release any KVM ISC registration and pinned NIB page,
> consistent with all other early-exit paths in the function.
> 
> 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 | 7 +++++++
>  1 file changed, 7 insertions(+)
> 
> diff --git a/drivers/s390/crypto/vfio_ap_ops.c b/drivers/s390/crypto/vfio_ap_ops.c
> index 6e4569d6b975..c7eebbd0ed40 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);
> @@ -1973,6 +1974,12 @@ static void apq_reset_check(struct work_struct *reset_work)
>  					      status.response_code,
>  					      status.queue_empty,
>  					      status.irq_enabled);
> +			if (elapsed >= AP_RESET_MAX_WAIT) {
> +				/* Timed out waiting for reset to complete */
> +				memcpy(&q->reset_status, &status, sizeof(status));
> +				vfio_ap_free_aqic_resources(q);
> +				return;

Sashiko points out a concern here and I tend to agree; if this timer
elapses you are effectively freeing resources that could still be in-use.

This seems to go back to dd174833e44e 's390/vfio-ap: remove upper limit
on wait for queue reset to complete' where it was decided to hang
forever vs leak resources -- e.g. the hang seems intentional?

If we don't have a way of forcing firmware to give up the resources I
think we are stuck either waiting indefinitely or quarantining (leaking)
the resources consciously.  And documenting the rationale in a comment
block.

> +			}
>  		} else {
>  			if (q->reset_status.response_code == AP_RESPONSE_RESET_IN_PROGRESS ||
>  			    q->reset_status.response_code == AP_RESPONSE_BUSY ||

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

[Matthew Rosato]

On 8/24/26 9:58 AM, Anthony Krowiak wrote:
> In vfio_ap_mdev_cfg_add(), the bitmap_and() call that computes
> the intersection of newly added control domains with those
> assigned to each matrix mdev passes AP_DEVICES as the bitmap
> size instead of AP_DOMAINS:
> 
>     bitmap_and(matrix_mdev->adm_add,
>                matrix_mdev->matrix.adm, adm_add, AP_DEVICES);
> 
> Control domains are indexed by AP queue index (APQI) and their
> bitmaps are sized by AP_DOMAINS, not AP_DEVICES. The two
> constants are both 256 today so there is no functional impact,
> but the wrong constant is inconsistent with the two bitmap_and()
> calls immediately above it and with every other operation on
> adm/aqm bitmaps in the file, all of which correctly use
> AP_DOMAINS.
> 
> Use AP_DOMAINS to keep the code consistent and correct in case
> the two constants ever diverge.
> 
> Signed-off-by: Anthony Krowiak <akrowiak@linux.ibm.com>

Since this is not a functional bug today (constants are the same value)
I'm OK with no fixes tag here.

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

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

[Anthony Krowiak]

On 8/24/26 12:57 PM, Matthew Rosato wrote:
> On 8/24/26 9:58 AM, Anthony Krowiak wrote:
>> The vfio_ap_irq_enable() function executes the PQAP(AQIC) instruction to
>> enable 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. 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.
>>
>> 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 | 11 ++++++++---
>>   1 file changed, 8 insertions(+), 3 deletions(-)
>>
>> diff --git a/drivers/s390/crypto/vfio_ap_ops.c b/drivers/s390/crypto/vfio_ap_ops.c
>> index 940c0ff668be..a46bf381ab72 100644
>> --- a/drivers/s390/crypto/vfio_ap_ops.c
>> +++ b/drivers/s390/crypto/vfio_ap_ops.c
>> @@ -503,9 +503,14 @@ 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);
>> +		pr_warn("%s: PQAP(AQIC) failed with response code %02x for apqn %04x\n",
>> +			__func__, status.response_code, q->apqn);
> LGTM, except Sashiko mentions the pr_warn here that you are updating has
> a pre-existing issue.
>
> Since you're touching it already, do you think it makes sense to switch
> to pr_warn_ratelimited with this patch?

I think it makes more sense to switch to VFIO_AP_DBF_WARN which all other
guest-triggered warning paths in  this function already use.

>
>> +		/* 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;
>>   	}
>>   

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

[Anthony Krowiak]

On 8/24/26 12:57 PM, Matthew Rosato wrote:
> On 8/24/26 9:58 AM, Anthony Krowiak wrote:
>> The vfio_ap_irq_enable() function executes the PQAP(AQIC) instruction to
>> enable 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. 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.
>>
>> 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 | 11 ++++++++---
>>   1 file changed, 8 insertions(+), 3 deletions(-)
>>
>> diff --git a/drivers/s390/crypto/vfio_ap_ops.c b/drivers/s390/crypto/vfio_ap_ops.c
>> index 940c0ff668be..a46bf381ab72 100644
>> --- a/drivers/s390/crypto/vfio_ap_ops.c
>> +++ b/drivers/s390/crypto/vfio_ap_ops.c
>> @@ -503,9 +503,14 @@ 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);
>> +		pr_warn("%s: PQAP(AQIC) failed with response code %02x for apqn %04x\n",
>> +			__func__, status.response_code, q->apqn);
> LGTM, except Sashiko mentions the pr_warn here that you are updating has
> a pre-existing issue.
>
> Since you're touching it already, do you think it makes sense to switch
> to pr_warn_ratelimited with this patch?

Since this is a pre-existing problem not introduced with this patch and 
there is
likewise another related problem not introduced by this patch, I am 
going to
post a separated series with fixes to those two related issues.

>
>> +		/* 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;
>>   	}
>>   

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

[Anthony Krowiak]

On 8/24/26 10:11 AM, sashiko-bot@kernel.org wrote:
> Thank you for your contribution! Sashiko AI review found 3 potential issue(s) to consider:
>
> Pre-existing issues:
> - [Critical] `vfio_ap_irq_enable` incorrectly treats `AP_RESPONSE_OTHERWISE_CHANGED` (0x07) as a failure, unpinning the NIB while the hardware has successfully configured it. This allows the AP hardware to DMA into freed host memory.
> - [High] Unprivileged guest can flood the host dmesg by repeatedly triggering a `pr_warn()` in the `default` case of the `ap_aqic()` response code switch.
> - [High] A malicious or misbehaving guest can trigger host-side WARN_ONCE macros by disabling IRQs on a checkstopped or busy AP queue, leading to kernel panics if panic_on_warn is enabled.
> --
>
> --- Patch [1]: [PATCH 1/4] s390/vfio-ap: Fix leak of pinned NIB and registered NISC in vfio_ap_irq_enable() ---
> commit c278a02f835924e412b00c7f6b715a44741cbcce
> Author: Anthony Krowiak <akrowiak@linux.ibm.com>
>
> s390/vfio-ap: Fix leak of pinned NIB and registered NISC in vfio_ap_irq_enable()
>
> This commit addresses a resource leak of pinned NIB and registered NISC
> pages during AP queue interrupt enablement. It updates the error handling
> path to unpin and unregister resources directly when an unexpected status
> is returned. This avoids incorrectly invoking the interrupt disable routine
> on a failing queue.
>
>> diff --git a/drivers/s390/crypto/vfio_ap_ops.c b/drivers/s390/crypto/vfio_ap_ops.c
>> index 940c0ff668bee..a46bf381ab726 100644
>> --- a/drivers/s390/crypto/vfio_ap_ops.c
>> +++ b/drivers/s390/crypto/vfio_ap_ops.c
>> @@ -503,9 +503,14 @@ static struct ap_queue_status vfio_ap_irq_enable(struct vfio_ap_queue *q,
>>   		vfio_unpin_pages(&q->matrix_mdev->vdev, nib, 1);
>>   		break;
> [Severity: Critical]
> This is a pre-existing issue, but does vfio_ap_irq_enable() incorrectly
> treat AP_RESPONSE_OTHERWISE_CHANGED (0x07) as a failure?

This is an interesting question. I took a look at the AP bus to see how this
response code is handled; it returns 0 from the function that processes the
PQAP(AQIC) call to enable interrupts, treating it as a successful 
completion.
This has generated several questions for which I'm seeking answers. 
Depending
on those answers, a new patch will be posted should this require a fix.

>
> When a guest triggers PQAP(AQIC) to enable interrupts on an AP queue that
> is not empty, the hardware returns AP_RESPONSE_OTHERWISE_CHANGED. This
> indicates successful IRQ modification but a non-empty queue.
>
> Because this code treats it as an error and unpins the NIB pages, couldn't
> the AP hardware later DMA interrupt data into the physical address of the
> unpinned NIB, corrupting host memory if those pages are reallocated?
>
>>   	default:
>> -		pr_warn("%s: apqn %04x: response: %02x\n", __func__, q->apqn,
>> -			status.response_code);
> [Severity: High]
> This isn't a bug introduced by this patch, but is it safe to leave this
> pr_warn() un-ratelimited in the default case?
>
> If an unprivileged KVM guest executes PQAP(AQIC) in a tight loop on a
> queue that is in a state returning an unhandled response code (e.g.,
> AP_RESPONSE_Q_NOT_AVAIL), couldn't they flood the host dmesg?
>
> This violates KVM subsystem rules which mandate that guest-reachable
> warnings must be rate-limited or removed to prevent log flood Denial of
> Service.

Looking at the existing code, all other guest-triggered warning paths in
this function already use VFIO_AP_DBF_WARN() — the default branch is the
odd one out. The fix is to drop the pr_warn entirely and use
VFIO_AP_DBF_WARN() which routes to the s390 debug facility (ring buffer)
and is not exposed to dmesg flooding. Since it wasn't introduced by this
patch, it will come with a separate series along with the fix to the comment
below which is for a similar problem.

>
>> -		vfio_ap_irq_disable(q);
> [Severity: High]
> This patch correctly removes the call to vfio_ap_irq_disable() here, but
> this highlights a pre-existing issue: is it possible for a malicious guest
> to trigger the WARN_ONCE macros inside vfio_ap_irq_disable() directly?
>
> If the guest executes PQAP(AQIC) with the interrupt enable bit set to 0
> (disable) on a queue that returns AP_RESPONSE_DECONFIGURED or
> AP_RESPONSE_CHECKSTOPPED, it will directly invoke vfio_ap_irq_disable():
>
> drivers/s390/crypto/vfio_ap_ops.c:vfio_ap_irq_disable() {
>      [ ... ]
> 	case AP_RESPONSE_Q_NOT_AVAIL:
> 	case AP_RESPONSE_DECONFIGURED:
> 	case AP_RESPONSE_CHECKSTOPPED:
> 	case AP_RESPONSE_INVALID_ADDRESS:
> 	default:
> 		/* All cases in default means AP not operational */
> 		WARN_ONCE(1, "%s: ap_aqic status %d\n", __func__,
> 			  status.response_code);
>      [ ... ]
> }
>
> Since this path is fully reachable by an unprivileged guest, couldn't this
> lead to a host kernel panic if panic_on_warn is enabled?

With panic_on_warn=1 this is a guest-triggerable host panic — a real DoS.
The fix is to replace the WARN_ONCE with VFIO_AP_DBF_WARN like that which
is used in the vfio_ap_irq_enable() function. The fix will be posted in 
a separate
series along with the fix mentioned above since neither was introduced via
this patch and are related.

>
>> +		pr_warn("%s: PQAP(AQIC) failed with response code %02x for apqn %04x\n",
>> +			__func__, status.response_code, q->apqn);
>> +		/* 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;
>>   	}

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

[Anthony Krowiak]

On 8/24/26 1:04 PM, Matthew Rosato wrote:
> On 8/24/26 9:58 AM, Anthony Krowiak wrote:
>> 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_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_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_TIMEOUT (2000ms) and breaking out
> s/AP_RESET_TIMEOUT/AP_RESET_MAX_WAIT/ ?

I had to change that string at one time because the compiler
complained about the AP_RESET_TIMEOUT being a duplicate.
I'll fix it.

>
>> 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. vfio_ap_free_aqic_resources() is called before returning
>> to release any KVM ISC registration and pinned NIB page,
>> consistent with all other early-exit paths in the function.
>>
>> 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 | 7 +++++++
>>   1 file changed, 7 insertions(+)
>>
>> diff --git a/drivers/s390/crypto/vfio_ap_ops.c b/drivers/s390/crypto/vfio_ap_ops.c
>> index 6e4569d6b975..c7eebbd0ed40 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);
>> @@ -1973,6 +1974,12 @@ static void apq_reset_check(struct work_struct *reset_work)
>>   					      status.response_code,
>>   					      status.queue_empty,
>>   					      status.irq_enabled);
>> +			if (elapsed >= AP_RESET_MAX_WAIT) {
>> +				/* Timed out waiting for reset to complete */
>> +				memcpy(&q->reset_status, &status, sizeof(status));
>> +				vfio_ap_free_aqic_resources(q);
>> +				return;
> Sashiko points out a concern here and I tend to agree; if this timer
> elapses you are effectively freeing resources that could still be in-use.
>
> This seems to go back to dd174833e44e 's390/vfio-ap: remove upper limit
> on wait for queue reset to complete' where it was decided to hang
> forever vs leak resources -- e.g. the hang seems intentional?
>
> If we don't have a way of forcing firmware to give up the resources I
> think we are stuck either waiting indefinitely or quarantining (leaking)
> the resources consciously.  And documenting the rationale in a comment
> block.
>
>> +			}
>>   		} else {
>>   			if (q->reset_status.response_code == AP_RESPONSE_RESET_IN_PROGRESS ||
>>   			    q->reset_status.response_code == AP_RESPONSE_BUSY ||

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

[Matthew Rosato]

>>> -        pr_warn("%s: apqn %04x: response: %02x\n", __func__, q->apqn,
>>> -            status.response_code);
>>> -        vfio_ap_irq_disable(q);
>>> +        pr_warn("%s: PQAP(AQIC) failed with response code %02x for
>>> apqn %04x\n",
>>> +            __func__, status.response_code, q->apqn);
>> LGTM, except Sashiko mentions the pr_warn here that you are updating has
>> a pre-existing issue.
>>
>> Since you're touching it already, do you think it makes sense to switch
>> to pr_warn_ratelimited with this patch?
> 
> Since this is a pre-existing problem not introduced with this patch and
> there is
> likewise another related problem not introduced by this patch, I am
> going to
> post a separated series with fixes to those two related issues.
> 
Isn't the change in pr_warn wording here also unnecessary/cosmetic?

Why don't you remove the change to pr_warn from this patch and re-word
it at the same time you convert it to VFIO_AP_DBF_WARN -- that way you
avoid touching/extending the pre-existing issue with this patch.

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

[Anthony Krowiak]

On 8/24/26 1:04 PM, Matthew Rosato wrote:
> On 8/24/26 9:58 AM, Anthony Krowiak wrote:
>> 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_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_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_TIMEOUT (2000ms) and breaking out
> s/AP_RESET_TIMEOUT/AP_RESET_MAX_WAIT/ ?
>
>> 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. vfio_ap_free_aqic_resources() is called before returning
>> to release any KVM ISC registration and pinned NIB page,
>> consistent with all other early-exit paths in the function.
>>
>> 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 | 7 +++++++
>>   1 file changed, 7 insertions(+)
>>
>> diff --git a/drivers/s390/crypto/vfio_ap_ops.c b/drivers/s390/crypto/vfio_ap_ops.c
>> index 6e4569d6b975..c7eebbd0ed40 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);
>> @@ -1973,6 +1974,12 @@ static void apq_reset_check(struct work_struct *reset_work)
>>   					      status.response_code,
>>   					      status.queue_empty,
>>   					      status.irq_enabled);
>> +			if (elapsed >= AP_RESET_MAX_WAIT) {
>> +				/* Timed out waiting for reset to complete */
>> +				memcpy(&q->reset_status, &status, sizeof(status));
>> +				vfio_ap_free_aqic_resources(q);
>> +				return;
> Sashiko points out a concern here and I tend to agree; if this timer
> elapses you are effectively freeing resources that could still be in-use.

Ironically, it was sashiko that precipitated this change given the
issue of hanging forever.

>
> This seems to go back to dd174833e44e 's390/vfio-ap: remove upper limit
> on wait for queue reset to complete' where it was decided to hang
> forever vs leak resources -- e.g. the hang seems intentional?

It may have been intentional, but I don't recall.

>
> If we don't have a way of forcing firmware to give up the resources I
> think we are stuck either waiting indefinitely or quarantining (leaking)
> the resources consciously.  And documenting the rationale in a comment
> block.

The AP architecture defines only a few instructions, none of which
provide a way to give up resources. I think it best to document this
in a comment block rather than waiting indefinitely. The
apq_reset_check() function is called under the matrix_dev->mdevs_lock
mutex which is a global lock that guards access to all active mdevs
in the system. Since the likelihood of this is happening is probably
extremely rare and the amount of storage leaked is not significant,
I think it makes more sense to allow things to proceed in this case.

>
>> +			}
>>   		} else {
>>   			if (q->reset_status.response_code == AP_RESPONSE_RESET_IN_PROGRESS ||
>>   			    q->reset_status.response_code == AP_RESPONSE_BUSY ||

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

[Anthony Krowiak]

On 8/24/26 3:56 PM, Matthew Rosato wrote:
>>>> -        pr_warn("%s: apqn %04x: response: %02x\n", __func__, q->apqn,
>>>> -            status.response_code);
>>>> -        vfio_ap_irq_disable(q);
>>>> +        pr_warn("%s: PQAP(AQIC) failed with response code %02x for
>>>> apqn %04x\n",
>>>> +            __func__, status.response_code, q->apqn);
>>> LGTM, except Sashiko mentions the pr_warn here that you are updating has
>>> a pre-existing issue.
>>>
>>> Since you're touching it already, do you think it makes sense to switch
>>> to pr_warn_ratelimited with this patch?
>> Since this is a pre-existing problem not introduced with this patch and
>> there is
>> likewise another related problem not introduced by this patch, I am
>> going to
>> post a separated series with fixes to those two related issues.
>>
> Isn't the change in pr_warn wording here also unnecessary/cosmetic?
>
> Why don't you remove the change to pr_warn from this patch and re-word
> it at the same time you convert it to VFIO_AP_DBF_WARN -- that way you
> avoid touching/extending the pre-existing issue with this patch.

Sure, will do.

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

[Anthony Krowiak]

On 8/24/26 3:56 PM, Matthew Rosato wrote:
>>>> -        pr_warn("%s: apqn %04x: response: %02x\n", __func__, q->apqn,
>>>> -            status.response_code);
>>>> -        vfio_ap_irq_disable(q);
>>>> +        pr_warn("%s: PQAP(AQIC) failed with response code %02x for
>>>> apqn %04x\n",
>>>> +            __func__, status.response_code, q->apqn);
>>> LGTM, except Sashiko mentions the pr_warn here that you are updating has
>>> a pre-existing issue.
>>>
>>> Since you're touching it already, do you think it makes sense to switch
>>> to pr_warn_ratelimited with this patch?
>> Since this is a pre-existing problem not introduced with this patch and
>> there is
>> likewise another related problem not introduced by this patch, I am
>> going to
>> post a separated series with fixes to those two related issues.
>>
> Isn't the change in pr_warn wording here also unnecessary/cosmetic?
>
> Why don't you remove the change to pr_warn from this patch and re-word
> it at the same time you convert it to VFIO_AP_DBF_WARN -- that way you
> avoid touching/extending the pre-existing issue with this patch.

Actually, now that I've looked at the entirety of the code, there is no
need to add a VFIO_AP_DBF_WARN here. After the switch statement
we have the following which duplicates the pr_warn but with more
detail:

if(status.response_code!= AP_RESPONSE_NORMAL) { 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", __func__, 
status.response_code, aqic_gisa.zone, aqic_gisa.ir, aqic_gisa.gisc, 
aqic_gisa.gf, aqic_gisa.gisa, aqic_gisa.isc, q->apqn);

}

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

[Anthony Krowiak]

On 8/24/26 10:11 AM, sashiko-bot@kernel.org wrote:
> Thank you for your contribution! Sashiko AI review found 1 potential issue(s) to consider:
>
> Pre-existing issues:
> - [High] Incomplete Fix / Latent Stack Buffer Overflow due to Inconsistent Constant Usage
> --
>
> --- Patch [4]: [PATCH 4/4] s390/vfio-ap: Use AP_DOMAINS for adm_add bitmap size in vfio_ap_mdev_cfg_add() ---
> commit 35cf2442d556c90b8b97a43b8f409fe51d21bae0
> Author: Anthony Krowiak <akrowiak@linux.ibm.com>
>
> s390/vfio-ap: Use AP_DOMAINS for adm_add bitmap size in vfio_ap_mdev_cfg_add()
>
> This commit changes the bitmap size constant from AP_DEVICES to AP_DOMAINS
> in vfio_ap_mdev_cfg_add() for the adm_add bitmap. The change prevents future
> issues if the constants' values ever diverge.
>
>>      Control domains are indexed by AP queue index (APQI) and their
>>      bitmaps are sized by AP_DOMAINS, not AP_DEVICES. The two
>>      constants are both 256 today so there is no functional impact,
>>      but the wrong constant is inconsistent with the two bitmap_and()
>>      calls immediately above it and with every other operation on
>>      adm/aqm bitmaps in the file, all of which correctly use
>>      AP_DOMAINS.
> [Severity: High]
> This is a pre-existing issue, but does every other operation correctly use
> AP_DOMAINS?
>
> Looking at vfio_ap_mdev_on_cfg_remove(), it appears AP_DEVICES is still used
> for domain masks sized by AP_DOMAINS:
>
> drivers/s390/crypto/vfio_ap_ops.c:vfio_ap_mdev_on_cfg_remove() {
> ...
> 	do_remove |= bitmap_andnot(aqrem,
> 				   (unsigned long *)prev_config_info->aqm,
> 				   (unsigned long *)cur_config_info->aqm,
> 				   AP_DEVICES);
> 	do_remove |= bitmap_andnot(cdrem,
> 				   (unsigned long *)prev_config_info->adm,
> 				   (unsigned long *)cur_config_info->adm,
> 				   AP_DEVICES);
> ...
> }
>
> Similarly, in vfio_ap_mdev_hot_unplug_domain():
>
> drivers/s390/crypto/vfio_ap_ops.c:vfio_ap_mdev_hot_unplug_domain() {
> ...
> 	DECLARE_BITMAP(apqis, AP_DOMAINS);
>
> 	bitmap_zero(apqis, AP_DEVICES);
> ...
> }
>
> If the constants AP_DEVICES and AP_DOMAINS ever diverge, could these
> operations cause out-of-bounds stack memory accesses?

Yes, those all need to be changed to use AP_DOMAINS

>