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

[PATCH v2 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.

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

Change log v1 => v2:
~~~~~~~~~~~~~~~~~~~
Patch 1/4: Fix leak of pinned NIB and registered NISC in 
           vfio_ap_irq_enable() 
* Removed call to pr_warn() in vfio_ap_irq_enable() for the default case
  in the switch statement to avoid flooding of the dmesg log. This was
  a pre-existing condition detected by sashiko and fixed here since it is
  in the code block changed by this patch. The VFIO_AP_DBF_WARN that occurs
  after the switch statement will display the error to the dbf log. 

Patch 3/4: Fix unbounded loop in apq_reset_check()
* Removed call to vfio_ap_free_aqic_resources() and provided a doc block
  explaining the rationale for allowing the leak of resources.

Patch 4/4: Use AP_DOMAINS for adm_add bitmap size in vfio_ap_mdev_cfg_add()
* Changed subject prefix to Use AP_DOMAINS for aqm/adm bitmap size
* Fixed two other locations where AP_DEVICES was used for the aqm/adm
  bitmap size.

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

-- 
2.53.0

[PATCH v2 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 | 9 ++++++---
 1 file changed, 6 insertions(+), 3 deletions(-)

diff --git a/drivers/s390/crypto/vfio_ap_ops.c b/drivers/s390/crypto/vfio_ap_ops.c
index 940c0ff668be..22a6ceaa7b56 100644
--- a/drivers/s390/crypto/vfio_ap_ops.c
+++ b/drivers/s390/crypto/vfio_ap_ops.c
@@ -503,9 +503,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;
 	}
 
-- 
2.53.0

Re: [PATCH v2 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 1 potential issue(s) to consider:

Pre-existing issues:
- [Critical] Potential host memory corruption if `vfio_ap_irq_disable` times out while the hardware queue is busy.
--

--- Patch [1]: [PATCH v2 1/4] s390/vfio-ap: Fix leak of pinned NIB and registered NISC in vfio_ap_irq_enable() ---
commit 85cfee5e61842e2a3b3b8993da6d14f25c2209a2
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 fixes a resource leak in vfio_ap_irq_enable() by explicitly
unregistering the NISC and unpinning the NIB in the default switch case,
rather than relying on vfio_ap_irq_disable().

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

[ ... ]

> @@ -503,9 +503,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);

[Severity: Critical]
This isn't a bug introduced by this patch, but pre-exists in the
vfio_ap_irq_disable() function that was previously called here.

Can this lead to host memory corruption if vfio_ap_irq_disable() times out
while the hardware queue is busy?

If a guest repeatedly sends requests to keep the AP queue busy, then issues a
PQAP(AQIC) instruction with the IR bit cleared (or unmaps memory),
vfio_ap_irq_disable() is invoked. If the queue remains busy, ap_aqic() returns
AP_RESPONSE_BUSY repeatedly.

After retries, vfio_ap_irq_disable() falls through to end_free:

drivers/s390/crypto/vfio_ap_ops.c:vfio_ap_irq_disable() {
	...
	} while (retries--);

	WARN_ONCE(1, "%s: ap_aqic status %d\n", __func__,
		  status.response_code);
end_free:
	vfio_ap_free_aqic_resources(q);
}

By unconditionally unpinning the NIB (Notification Indicator Byte) page via
vfio_ap_free_aqic_resources() while the hardware still holds its address and
expects to write to it, the page may be reallocated to another process.

When the hardware eventually triggers an interrupt, wouldn't it perform a DMA
write to this freed page, causing host memory corruption?

> +		/* 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/20260824215603.819379-1-akrowiak@linux.ibm.com?part=1

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

[Anthony Krowiak]

On 8/24/26 6:07 PM, sashiko-bot@kernel.org wrote:
> Thank you for your contribution! Sashiko AI review found 1 potential issue(s) to consider:
>
> Pre-existing issues:
> - [Critical] Potential host memory corruption if `vfio_ap_irq_disable` times out while the hardware queue is busy.
> --
>
> --- Patch [1]: [PATCH v2 1/4] s390/vfio-ap: Fix leak of pinned NIB and registered NISC in vfio_ap_irq_enable() ---
> commit 85cfee5e61842e2a3b3b8993da6d14f25c2209a2
> 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 fixes a resource leak in vfio_ap_irq_enable() by explicitly
> unregistering the NISC and unpinning the NIB in the default switch case,
> rather than relying on vfio_ap_irq_disable().
>
>> diff --git a/drivers/s390/crypto/vfio_ap_ops.c b/drivers/s390/crypto/vfio_ap_ops.c
>> index 940c0ff668bee..22a6ceaa7b562 100644
>> --- a/drivers/s390/crypto/vfio_ap_ops.c
>> +++ b/drivers/s390/crypto/vfio_ap_ops.c
> [ ... ]
>
>> @@ -503,9 +503,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);
> [Severity: Critical]
> This isn't a bug introduced by this patch, but pre-exists in the
> vfio_ap_irq_disable() function that was previously called here.
>
> Can this lead to host memory corruption if vfio_ap_irq_disable() times out
> while the hardware queue is busy?
>
> If a guest repeatedly sends requests to keep the AP queue busy, then issues a
> PQAP(AQIC) instruction with the IR bit cleared (or unmaps memory),
> vfio_ap_irq_disable() is invoked. If the queue remains busy, ap_aqic() returns
> AP_RESPONSE_BUSY repeatedly.
>
> After retries, vfio_ap_irq_disable() falls through to end_free:
>
> drivers/s390/crypto/vfio_ap_ops.c:vfio_ap_irq_disable() {
> 	...
> 	} while (retries--);
>
> 	WARN_ONCE(1, "%s: ap_aqic status %d\n", __func__,
> 		  status.response_code);
> end_free:
> 	vfio_ap_free_aqic_resources(q);
> }
>
> By unconditionally unpinning the NIB (Notification Indicator Byte) page via
> vfio_ap_free_aqic_resources() while the hardware still holds its address and
> expects to write to it, the page may be reallocated to another process.
>
> When the hardware eventually triggers an interrupt, wouldn't it perform a DMA
> write to this freed page, causing host memory corruption?

This is a valid concern and is similar to the problem fixed by this patch.
It was also introduced by the same patch identified in the Fixes: tag,
so I will add the fix for the issue pointed out above to this patch and
resubmit it.

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

[PATCH v2 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 22a6ceaa7b56..3f99b239fa95 100644
--- a/drivers/s390/crypto/vfio_ap_ops.c
+++ b/drivers/s390/crypto/vfio_ap_ops.c
@@ -2064,12 +2064,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 v2 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/20260824215603.819379-1-akrowiak@linux.ibm.com?part=2

[PATCH v2 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. 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 | 22 ++++++++++++++++++++++
 1 file changed, 22 insertions(+)

diff --git a/drivers/s390/crypto/vfio_ap_ops.c b/drivers/s390/crypto/vfio_ap_ops.c
index 3f99b239fa95..7a9b5448e90a 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);
@@ -1971,6 +1972,27 @@ 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.
+				 *
+				 * 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:
+				 * apq_reset_check() 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;
+			}
 		} else {
 			if (q->reset_status.response_code == AP_RESPONSE_RESET_IN_PROGRESS ||
 			    q->reset_status.response_code == AP_RESPONSE_BUSY ||
-- 
2.53.0

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

[sashiko-bot]

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

New issues:
- [Critical] The timeout mechanism intentionally skips unpinning the NIB page to avoid a driver-level UAF, but fails to realize the VFIO core will automatically unpin and free the page when the device is closed, resulting in a critical hardware DMA Use-After-Free.
- [Critical] The timeout handler leaves `q->saved_iova` and `q->saved_isc` populated with the old guest's state, leading to cross-guest resource corruption when the queue is assigned to a new mdev.
- [High] The timeout error path deliberately leaks the registered guest ISC and pinned NIB page, which permanently exhausts ISCs and directly causes a host kernel WARN when the KVM guest terminates.
- [High] The worker can still loop indefinitely because the new timeout check is bypassed in the 'else' branch when retrying the reset via ap_zapq().
- [Medium] The commit message explicitly claims that AQIC resources are freed on the early exit path, but the code does the exact opposite.

Pre-existing issues:
- [High] The worker returns early on `-EIO` without writing the final hardware status back to `q->reset_status`.
--

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

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

This commit introduces a maximum wait timeout in the apq_reset_check worker
to prevent indefinite loops when the hardware continuously returns a busy
response. On timeout, the function writes the busy status back and breaks out
of the loop.

> 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. 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.

[Severity: Medium]
Is this description accurate? The code in the patch explicitly omits calling
vfio_ap_free_aqic_resources() on the timeout path and includes a large
comment explaining why it must leak these resources.

> diff --git a/drivers/s390/crypto/vfio_ap_ops.c b/drivers/s390/crypto/vfio_ap_ops.c
> index 3f99b239fa959..7a9b5448e90a0 100644
> --- a/drivers/s390/crypto/vfio_ap_ops.c
> +++ b/drivers/s390/crypto/vfio_ap_ops.c
[ ... ]
> @@ -1971,6 +1972,27 @@ static void apq_reset_check(struct work_struct *reset_work)
>  					      status.response_code,
>  					      status.queue_empty,
>  					      status.irq_enabled);

[Severity: High]
This is a pre-existing issue, but does the early return on -EIO just above
this block need to update q->reset_status before returning?

If ap_zapq() initially succeeded, q->reset_status.response_code would be 0.
If a subsequent ap_tapq() results in an -EIO from apq_status_check(), the
function returns immediately. Will the caller vfio_ap_mdev_reset_queues()
incorrectly assume the reset was successful because it sees a 0 response code?

> +			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.

[Severity: Critical]
The comment mentions leaking the NIB page to avoid a use-after-free, but does
the driver hold an independent reference to the page?

When the user process terminates and the VFIO container is closed, the VFIO
core automatically unpins tracked pages. Will the physical page be returned to
the buddy allocator while the active hardware can still perform a DMA write to
it, potentially corrupting memory reallocated to other processes?

> +				 *
> +				 * If the reset eventually completes, interrupts will be terminated
> +				 * and the pinned NIB page and ISC registration will be leaked. This

[Severity: High]
By intentionally skipping vfio_ap_free_aqic_resources(),
kvm_s390_gisc_unregister() is never called. Does this mean KVM ISCs are
permanently depleted for the guest?

Also, when the KVM guest is torn down, will kvm_s390_gisa_destroy() trigger a
host kernel WARN due to the non-zero alert mask?

> +				 * is preferable to either a use-after-free or waiting indefinitely:
> +				 * apq_reset_check() 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: Critical]
If the timeout handler leaves q->saved_iova and q->saved_isc populated, what
happens when this queue is assigned to a new mdev?

When the new guest enables interrupts, vfio_ap_free_aqic_resources() will
process the old guest's saved IOVA and ISC using the new guest's KVM and
VFIO container. Could this unpin an active page in the new guest or corrupt
its GISA state?

>  		} 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]
Does this bypass the timeout check entirely if apq_status_check() returns
-EAGAIN?

The loop will drop into this else block, re-issue ap_zapq(), update the
status, and continue without evaluating the elapsed time. Will the worker
spin indefinitely if the hardware continuously returns a state that triggers
-EAGAIN?

>  			if (q->saved_isc != VFIO_AP_ISC_INVALID)
>  				vfio_ap_free_aqic_resources(q);
>  			break;
>  		}

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

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

[Anthony Krowiak]

On 8/24/26 6:11 PM, sashiko-bot@kernel.org wrote:
> Thank you for your contribution! Sashiko AI review found 6 potential issue(s) to consider:
>
> New issues:
> - [Critical] The timeout mechanism intentionally skips unpinning the NIB page to avoid a driver-level UAF, but fails to realize the VFIO core will automatically unpin and free the page when the device is closed, resulting in a critical hardware DMA Use-After-Free.
> - [Critical] The timeout handler leaves `q->saved_iova` and `q->saved_isc` populated with the old guest's state, leading to cross-guest resource corruption when the queue is assigned to a new mdev.
> - [High] The timeout error path deliberately leaks the registered guest ISC and pinned NIB page, which permanently exhausts ISCs and directly causes a host kernel WARN when the KVM guest terminates.
> - [High] The worker can still loop indefinitely because the new timeout check is bypassed in the 'else' branch when retrying the reset via ap_zapq().
> - [Medium] The commit message explicitly claims that AQIC resources are freed on the early exit path, but the code does the exact opposite.
>
> Pre-existing issues:
> - [High] The worker returns early on `-EIO` without writing the final hardware status back to `q->reset_status`.
> --
>
> --- Patch [3]: [PATCH v2 3/4] s390/vfio-ap: Fix unbounded loop in apq_reset_check() ---
> commit 84304c1f304fd7bd597c0aa370c7ee8ddc904d98
> Author: Anthony Krowiak <akrowiak@linux.ibm.com>
>
> s390/vfio-ap: Fix unbounded loop in apq_reset_check()
>
> This commit introduces a maximum wait timeout in the apq_reset_check worker
> to prevent indefinite loops when the hardware continuously returns a busy
> response. On timeout, the function writes the busy status back and breaks out
> of the loop.
>
>> 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. 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.
> [Severity: Medium]
> Is this description accurate? The code in the patch explicitly omits calling
> vfio_ap_free_aqic_resources() on the timeout path and includes a large
> comment explaining why it must leak these resources.

The description needs to change to match the code change.

>
>> diff --git a/drivers/s390/crypto/vfio_ap_ops.c b/drivers/s390/crypto/vfio_ap_ops.c
>> index 3f99b239fa959..7a9b5448e90a0 100644
>> --- a/drivers/s390/crypto/vfio_ap_ops.c
>> +++ b/drivers/s390/crypto/vfio_ap_ops.c
> [ ... ]
>> @@ -1971,6 +1972,27 @@ static void apq_reset_check(struct work_struct *reset_work)
>>   					      status.response_code,
>>   					      status.queue_empty,
>>   					      status.irq_enabled);
> [Severity: High]
> This is a pre-existing issue, but does the early return on -EIO just above
> this block need to update q->reset_status before returning?

The response status returned from the tapq needs to be co

>
> If ap_zapq() initially succeeded, q->reset_status.response_code would be 0.
> If a subsequent ap_tapq() results in an -EIO from apq_status_check(), the
> function returns immediately. Will the caller vfio_ap_mdev_reset_queues()
> incorrectly assume the reset was successful because it sees a 0 response code?
>
>> +			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.
> [Severity: Critical]
> The comment mentions leaking the NIB page to avoid a use-after-free, but does
> the driver hold an independent reference to the page?
>
> When the user process terminates and the VFIO container is closed, the VFIO
> core automatically unpins tracked pages. Will the physical page be returned to
> the buddy allocator while the active hardware can still perform a DMA write to
> it, potentially corrupting memory reallocated to other processes?

Pages pinned via vfio_pin_pages() are reference-counted by the VFIO layer
and are not automatically unpinned unless the driver calls
vfio_unpin_pages(). The page is genuinely leaked (intentionally), not
silently freed.

The driver does hold an independent reference to the page, so it should
freed when the kvm is torn down, the mdev is removed, a queue is
removed from the device driver or a queue is unassigned from the mdev.
This will be fixed in a forthcoming patch.

>
>> +				 *
>> +				 * If the reset eventually completes, interrupts will be terminated
>> +				 * and the pinned NIB page and ISC registration will be leaked. This
> [Severity: High]
> By intentionally skipping vfio_ap_free_aqic_resources(),
> kvm_s390_gisc_unregister() is never called. Does this mean KVM ISCs are
> permanently depleted for the guest?
>
> Also, when the KVM guest is torn down, will kvm_s390_gisa_destroy() trigger a
> host kernel WARN due to the non-zero alert mask?

This is a legitimate concern. Similar to my response above, this will be
fixed in a forthcoming patch.

>
>> +				 * is preferable to either a use-after-free or waiting indefinitely:
>> +				 * apq_reset_check() 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: Critical]
> If the timeout handler leaves q->saved_iova and q->saved_isc populated, what
> happens when this queue is assigned to a new mdev?
>
> When the new guest enables interrupts, vfio_ap_free_aqic_resources() will
> process the old guest's saved IOVA and ISC using the new guest's KVM and
> VFIO container. Could this unpin an active page in the new guest or corrupt
> its GISA state?

This is a legitimate concern that will be fixed by clearing these resources
before any queue can get assigned to another mdev (see above responses).


>
>>   		} 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]
> Does this bypass the timeout check entirely if apq_status_check() returns
> -EAGAIN?
>
> The loop will drop into this else block, re-issue ap_zapq(), update the
> status, and continue without evaluating the elapsed time. Will the worker
> spin indefinitely if the hardware continuously returns a state that triggers
> -EAGAIN?

Yes. This will be fixed in v3.

>
>>   			if (q->saved_isc != VFIO_AP_ISC_INVALID)
>>   				vfio_ap_free_aqic_resources(q);
>>   			break;
>>   		}

[PATCH v2 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 7a9b5448e90a..a41d55146db1 100644
--- a/drivers/s390/crypto/vfio_ap_ops.c
+++ b/drivers/s390/crypto/vfio_ap_ops.c
@@ -1414,7 +1414,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);
 }
@@ -2738,11 +2738,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);
@@ -2853,7 +2853,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 v2 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/20260824215603.819379-1-akrowiak@linux.ibm.com?part=4