[PATCH v5 0/9] s390/vfio-ap: Fix bugs in vfio_ap device driver callback functions

[PATCH v5 0/9] s390/vfio-ap: Fix bugs in vfio_ap device driver callback functions

[Anthony Krowiak]

During review of patches by the Sashiko AI, several pre-existing bugs were
discovered. This 9-patch series fixes those bugs.

Change log v4 => v5:
~~~~~~~~~~~~~~~~~~~
Patch 8/9: Fix NULL deref in status_show() during queue probe
* Check if q = dev_get_drvdata(&apdev->device) actually returns a pointer
  to a vfio_ap_queue object. If not, the return AP_QUEUE_UNASSIGNED

Anthony Krowiak (9):
  s390/vfio-ap: Fix stale do_remove flag across iterations in
    vfio_ap_mdev_cfg_remove
  s390/vfio-ap: Fix dereference matrix_mdev->kvm without checking for
    NULL
  s390/vfio-ap: Fix missing lock required to access list of
    ap_matrix_mdev objects
  s390/vfio-ap: Fix required lock not held during update of
    ap_matrix_mdev object
  s390/vfio-ap: Fix control domain removal in vfio_ap_mdev_cfg_remove
  s390/vfio-ap: fix potential use of uninitialized apm_filtered bitmap
  s390/vfio-ap: Fix hot-unplug skipped when last AP adapter or domain
    removed
  s390/vfio-ap: Fix NULL deref in status_show() during queue probe
  s390/vfio-ap: Fix memory leak when queue removed from host AP config

 drivers/s390/crypto/vfio_ap_ops.c | 163 +++++++++++++++++++++++-------
 1 file changed, 126 insertions(+), 37 deletions(-)

-- 
2.53.0

[PATCH v5 1/9] s390/vfio-ap: Fix stale do_remove flag across iterations in vfio_ap_mdev_cfg_remove

[Anthony Krowiak]

The do_remove flag in vfio_ap_mdev_cfg_remove() is initialised to zero
before the loop that iterates over the list of matrix mdevs, but is
never reset at the start of each iteration. Since do_remove is
OR-accumulated across iterations, a positive result from one mdev
carries over to subsequent mdevs.

The fix is to set the do_remove flag with the first call to bitmap_and;
for example: do_remove = bitmap_an rather than do_remove |= bitmap_and.

Fixes: eeb386aeb5b7 ("s390/vfio-ap: handle config changed and scan complete notification")
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 | 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 44b3a1dcc1b3..845c86ba8bc3 100644
--- a/drivers/s390/crypto/vfio_ap_ops.c
+++ b/drivers/s390/crypto/vfio_ap_ops.c
@@ -2603,15 +2603,15 @@ static void vfio_ap_mdev_cfg_remove(unsigned long *ap_remove,
 	DECLARE_BITMAP(aprem, AP_DEVICES);
 	DECLARE_BITMAP(aqrem, AP_DOMAINS);
 	DECLARE_BITMAP(cdrem, AP_DOMAINS);
-	int do_remove = 0;
+	int do_remove;
 
 	list_for_each_entry(matrix_mdev, &matrix_dev->mdev_list, node) {
 		mutex_lock(&matrix_mdev->kvm->lock);
 		mutex_lock(&matrix_dev->mdevs_lock);
 
-		do_remove |= bitmap_and(aprem, ap_remove,
-					  matrix_mdev->matrix.apm,
-					  AP_DEVICES);
+		do_remove = bitmap_and(aprem, ap_remove,
+				       matrix_mdev->matrix.apm,
+				       AP_DEVICES);
 		do_remove |= bitmap_and(aqrem, aq_remove,
 					  matrix_mdev->matrix.aqm,
 					  AP_DOMAINS);
-- 
2.53.0

Re: [PATCH v5 1/9] s390/vfio-ap: Fix stale do_remove flag across iterations in vfio_ap_mdev_cfg_remove

[sashiko-bot]

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

Pre-existing issues:
- [High] Unconditional dereference of `matrix_mdev->kvm` causes kernel panics for mdevs not yet attached to a guest.
- [High] Incorrect use of `bitmap_andnot` instead of `bitmap_and` prevents removed control domains from being hot-unplugged from guests.
- [High] Unprotected iteration of `matrix_dev->mdev_list` leads to potential data races and list corruption.
--

--- Patch [1]: [PATCH v5 1/9] s390/vfio-ap: Fix stale do_remove flag across iterations in vfio_ap_mdev_cfg_remove ---
commit 62f5d072f498c6d3b4f1ad645dbc6b551ae4f363
Author: Anthony Krowiak <akrowiak@linux.ibm.com>

s390/vfio-ap: Fix stale do_remove flag across iterations in vfio_ap_mdev_cfg_remove

The do_remove flag in vfio_ap_mdev_cfg_remove() is initialised to zero
before the loop that iterates over the list of matrix mdevs, but is
never reset at the start of each iteration. Since do_remove is
OR-accumulated across iterations, a positive result from one mdev
carries over to subsequent mdevs.

The fix is to set the do_remove flag with the first call to bitmap_and;
for example: do_remove = bitmap_an rather than do_remove |= bitmap_and.

> diff --git a/drivers/s390/crypto/vfio_ap_ops.c b/drivers/s390/crypto/vfio_ap_ops.c
> index 44b3a1dcc1b36..845c86ba8bc30 100644
> --- a/drivers/s390/crypto/vfio_ap_ops.c
> +++ b/drivers/s390/crypto/vfio_ap_ops.c
> @@ -2603,15 +2603,15 @@ static void vfio_ap_mdev_cfg_remove(unsigned long *ap_remove,
>  	DECLARE_BITMAP(aprem, AP_DEVICES);
>  	DECLARE_BITMAP(aqrem, AP_DOMAINS);
>  	DECLARE_BITMAP(cdrem, AP_DOMAINS);
> -	int do_remove = 0;
> +	int do_remove;
>  
>  	list_for_each_entry(matrix_mdev, &matrix_dev->mdev_list, node) {

[Severity: High]
This isn't a bug introduced by this patch, but does iterating
matrix_dev->mdev_list here require holding matrix_dev->mdevs_lock?

Looking at vfio_ap_mdev_probe(), it appears to add new mdevs to this list via
list_add() while holding only matrix_dev->mdevs_lock.

Since this loop is entered before acquiring matrix_dev->mdevs_lock, can an
mdev creation running concurrently with an AP configuration change modify the
list while it is being iterated, potentially leading to a data race or list
corruption?

>  		mutex_lock(&matrix_mdev->kvm->lock);

[Severity: High]
This is a pre-existing issue, but could this unconditionally dereference a
NULL kvm pointer?

If a user creates a mediated device and assigns AP resources to it without
attaching it to a KVM guest, matrix_mdev->kvm would be NULL.

If a host AP configuration change occurs, would dereferencing matrix_mdev->kvm
here cause a kernel panic?

This same pattern appears in vfio_ap_mdev_hot_plug_cfg():

drivers/s390/crypto/vfio_ap_ops.c:vfio_ap_mdev_hot_plug_cfg() {
    ...
	list_for_each_entry(matrix_mdev, &matrix_dev->mdev_list, node) {
		mutex_lock(&matrix_mdev->kvm->lock);
    ...
}

Does this exhibit the same NULL pointer dereference risk when an mdev is not
yet attached to a guest?

>  		mutex_lock(&matrix_dev->mdevs_lock);
>  
> -		do_remove |= bitmap_and(aprem, ap_remove,
> -					  matrix_mdev->matrix.apm,
> -					  AP_DEVICES);
> +		do_remove = bitmap_and(aprem, ap_remove,
> +				       matrix_mdev->matrix.apm,
> +				       AP_DEVICES);
>  		do_remove |= bitmap_and(aqrem, aq_remove,
>  					  matrix_mdev->matrix.aqm,
>  					  AP_DOMAINS);

[Severity: High]
This is a pre-existing issue, but is bitmap_andnot() the correct operation for
calculating cdrem just below this?

drivers/s390/crypto/vfio_ap_ops.c:vfio_ap_mdev_cfg_remove() {
    ...
		do_remove |= bitmap_andnot(cdrem, cd_remove,
					     matrix_mdev->matrix.adm,
					     AP_DOMAINS);
    ...
}

This computes the cdrem bitmask using inverted logic, isolating removed
domains that are NOT assigned to the mdev.

Should this use bitmap_and() like the aprem and aqrem calculations, so that
assigned control domains are properly identified and hot-unplugged from
guests?

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

[PATCH v5 2/9] s390/vfio-ap: Fix dereference matrix_mdev->kvm without checking for NULL

[Anthony Krowiak]

The ap_driver structure has two fields which are function pointers to
callbacks:

* .on_config_changed: called at the start of the AP bus scan function to
                      notify the device driver that the host AP
                      configuration has changed and the associated AP
                      devices will be added or removed accordingly. This
                      gives the implementor a chance to evaluate the
                      configuration changes and respond to them before
                      the associated devices are added or removed.

* .on_scan_complete:  Called at the end of the AP bus scan function to
                      notify the device driver that the host AP
                      configuration has changed and the AP devices have
                      been added or removed accordingly. This gives the
                      implementor the opportunity to respond to the
                      changes after the associated devices are added or
                      removed.

These two callbacks are implemented in the vfio_ap device driver via the
vfio_ap_on_cfg_changed and vfio_ap_on_scan_complete functions respectively.

Within the call stack of these two callback functions the
matrix_mdev->kvm->lock mutex is taken without checking whether
matrix_mdev->kvm is NULL or not. If matrix_mdev->kvm has never been set,
trying to take the lock will trigger a NULL pointer dereference. This patch
adds checks for matrix_mdev->kvm == NULL before taking the
matrix_mdev->kvm->lock mutex.

Note that the matrix_mdev->kvm->lock mutex taken in the
vfio_ap_mdev_hot_plug_config function is moved to the calling function
along with the matrix_dev->mdevs_lock which is needed there to access
the fields of the matrix_mdev. It makes little sense to make the change
the check for matrix_mdev->kvm there before taking the kvm->lock
mutex only to have to move it out via another patch, so it is done in
this patch.

It is important to make note of the following:
1. The matrix_dev->guests_lock is acquired at the start of both callback
   functions. This ensures that matrix_mdev will not be removed via the
   vfio_ap_mdev_remove function because it too takes matrix_dev_guests_lock
   before removing the object; so, matrix_mdev will be available for the
   duration of the callback functions.

2. The matrix_dev->mdevs_lock mutex must be taken in order to access
   fields within the matrix_mdev structure

3. matrix_mdev->kvm->lock mutex must be taken before the
   matrix_dev->mdevs_lock to prevent a lockdep splat.

4: The kvm->lock must be held while plugging the guest's AP configuration
   into its SIE state description via the vfio_ap_mdev_update_guest_apcb
   function.

5. The vfio_ap_mdev_update_guest_apcb checks matrix_mdev->kvm to verify it
   is not NULL before doing the hot plug of the guest's AP configuration.

Fixes: eeb386aeb5b7c ("s390/vfio-ap: handle config changed and scan complete notification")
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 | 39 ++++++++++++++++++++++++-------
 1 file changed, 30 insertions(+), 9 deletions(-)

diff --git a/drivers/s390/crypto/vfio_ap_ops.c b/drivers/s390/crypto/vfio_ap_ops.c
index 845c86ba8bc3..c6bee69cc22f 100644
--- a/drivers/s390/crypto/vfio_ap_ops.c
+++ b/drivers/s390/crypto/vfio_ap_ops.c
@@ -2605,8 +2605,20 @@ static void vfio_ap_mdev_cfg_remove(unsigned long *ap_remove,
 	DECLARE_BITMAP(cdrem, AP_DOMAINS);
 	int do_remove;
 
+	/*
+	 * It is safe to traverse this list here because the
+	 * required guard - matrix_dev->guests_lock - is taken in the
+	 * vfio_ap_on_cfg_changed function prior to this function getting
+	 * called.
+	 */
 	list_for_each_entry(matrix_mdev, &matrix_dev->mdev_list, node) {
-		mutex_lock(&matrix_mdev->kvm->lock);
+		/*
+		 * The mdevs_lock must be held to access fields within matrix_mdev,
+		 * and kvm->lock must be taken before mdevs_lock to satisfy the lock
+		 * ordering requirement and prevent a lockdep splat.
+		 */
+		if (matrix_mdev->kvm)
+			mutex_lock(&matrix_mdev->kvm->lock);
 		mutex_lock(&matrix_dev->mdevs_lock);
 
 		do_remove = bitmap_and(aprem, ap_remove,
@@ -2624,7 +2636,8 @@ static void vfio_ap_mdev_cfg_remove(unsigned long *ap_remove,
 						    cdrem);
 
 		mutex_unlock(&matrix_dev->mdevs_lock);
-		mutex_unlock(&matrix_mdev->kvm->lock);
+		if (matrix_mdev->kvm)
+			mutex_unlock(&matrix_mdev->kvm->lock);
 	}
 }
 
@@ -2821,9 +2834,6 @@ static void vfio_ap_mdev_hot_plug_cfg(struct ap_matrix_mdev *matrix_mdev)
 	DECLARE_BITMAP(apm_filtered, AP_DEVICES);
 	bool filter_domains, filter_adapters, filter_cdoms, do_hotplug = false;
 
-	mutex_lock(&matrix_mdev->kvm->lock);
-	mutex_lock(&matrix_dev->mdevs_lock);
-
 	filter_adapters = bitmap_intersects(matrix_mdev->matrix.apm,
 					    matrix_mdev->apm_add, AP_DEVICES);
 	filter_domains = bitmap_intersects(matrix_mdev->matrix.aqm,
@@ -2841,9 +2851,6 @@ static void vfio_ap_mdev_hot_plug_cfg(struct ap_matrix_mdev *matrix_mdev)
 		vfio_ap_mdev_update_guest_apcb(matrix_mdev);
 
 	reset_queues_for_apids(matrix_mdev, apm_filtered);
-
-	mutex_unlock(&matrix_dev->mdevs_lock);
-	mutex_unlock(&matrix_mdev->kvm->lock);
 }
 
 void vfio_ap_on_scan_complete(struct ap_config_info *new_config_info,
@@ -2854,15 +2861,29 @@ void vfio_ap_on_scan_complete(struct ap_config_info *new_config_info,
 	mutex_lock(&matrix_dev->guests_lock);
 
 	list_for_each_entry(matrix_mdev, &matrix_dev->mdev_list, node) {
+		/*
+		 * The mdevs_lock must be held to access fields within matrix_mdev,
+		 * and kvm->lock must be taken before mdevs_lock to satisfy the lock
+		 * ordering requirement and prevent a lockdep splat.
+		 */
+		if (matrix_mdev->kvm)
+			mutex_lock(&matrix_mdev->kvm->lock);
+		mutex_lock(&matrix_dev->mdevs_lock);
+
 		if (bitmap_empty(matrix_mdev->apm_add, AP_DEVICES) &&
 		    bitmap_empty(matrix_mdev->aqm_add, AP_DOMAINS) &&
 		    bitmap_empty(matrix_mdev->adm_add, AP_DOMAINS))
-			continue;
+			goto do_unlock;
 
 		vfio_ap_mdev_hot_plug_cfg(matrix_mdev);
 		bitmap_clear(matrix_mdev->apm_add, 0, AP_DEVICES);
 		bitmap_clear(matrix_mdev->aqm_add, 0, AP_DOMAINS);
 		bitmap_clear(matrix_mdev->adm_add, 0, AP_DOMAINS);
+
+do_unlock:
+		mutex_unlock(&matrix_dev->mdevs_lock);
+		if (matrix_mdev->kvm)
+			mutex_unlock(&matrix_mdev->kvm->lock);
 	}
 
 	mutex_unlock(&matrix_dev->guests_lock);
-- 
2.53.0

Re: [PATCH v5 2/9] s390/vfio-ap: Fix dereference matrix_mdev->kvm without checking for NULL

[sashiko-bot]

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

Pre-existing issues:
- [High] The `apm_filtered` stack array is conditionally left uninitialized but unconditionally used, causing random AP queue resets.
- [High] Concurrent list additions and traversals cause unprotected data races on `matrix_dev->mdev_list`.
--

--- Patch [2]: [PATCH v5 2/9] s390/vfio-ap: Fix dereference matrix_mdev->kvm without checking for NULL ---
commit da7c08a88a7102b1daed33f3e6352fe4dffb203f
Author: Anthony Krowiak <akrowiak@linux.ibm.com>

s390/vfio-ap: Fix dereference matrix_mdev->kvm without checking for NULL

This patch adds checks to ensure matrix_mdev->kvm is not NULL before taking
the matrix_mdev->kvm->lock mutex during the configuration change and scan
complete callbacks. The locking order between kvm->lock and mdevs_lock is
also updated.

> diff --git a/drivers/s390/crypto/vfio_ap_ops.c b/drivers/s390/crypto/vfio_ap_ops.c
> index 845c86ba8bc30..c6bee69cc22f6 100644
> --- a/drivers/s390/crypto/vfio_ap_ops.c
> +++ b/drivers/s390/crypto/vfio_ap_ops.c
[ ... ]
> @@ -2821,9 +2834,6 @@ static void vfio_ap_mdev_hot_plug_cfg(struct ap_matrix_mdev *matrix_mdev)
>  	DECLARE_BITMAP(apm_filtered, AP_DEVICES);
>  	bool filter_domains, filter_adapters, filter_cdoms, do_hotplug = false;
>  
> -	mutex_lock(&matrix_mdev->kvm->lock);
> -	mutex_lock(&matrix_dev->mdevs_lock);
> -
>  	filter_adapters = bitmap_intersects(matrix_mdev->matrix.apm,
>  					    matrix_mdev->apm_add, AP_DEVICES);
>  	filter_domains = bitmap_intersects(matrix_mdev->matrix.aqm,
[ ... ]
> @@ -2841,9 +2851,6 @@ static void vfio_ap_mdev_hot_plug_cfg(struct ap_matrix_mdev *matrix_mdev)
>  		vfio_ap_mdev_update_guest_apcb(matrix_mdev);
>  
>  	reset_queues_for_apids(matrix_mdev, apm_filtered);

[Severity: High]
This isn't a bug introduced by this patch, but can this sequence result in
reading uninitialized stack memory?

In vfio_ap_mdev_hot_plug_cfg(), DECLARE_BITMAP() allocates apm_filtered on
the stack but does not initialize it.

If filter_adapters and filter_domains are both false (for example, if only
control domains were added), the code bypasses vfio_ap_mdev_filter_matrix(),
leaving the stack array completely uninitialized.

When reset_queues_for_apids() is then called, will it unconditionally iterate
over this garbage memory and potentially trigger unintended hardware queue
resets (ap_zapq) for random matching queues?

> -
> -	mutex_unlock(&matrix_dev->mdevs_lock);
> -	mutex_unlock(&matrix_mdev->kvm->lock);
>  }
>  
>  void vfio_ap_on_scan_complete(struct ap_config_info *new_config_info,
> @@ -2854,15 +2861,29 @@ void vfio_ap_on_scan_complete(struct ap_config_info *new_config_info,
>  	mutex_lock(&matrix_dev->guests_lock);
>  
>  	list_for_each_entry(matrix_mdev, &matrix_dev->mdev_list, node) {

[Severity: High]
This is a pre-existing issue, but does this list traversal have an
unprotected data race?

The matrix_dev->mdev_list is traversed here holding only
matrix_dev->guests_lock. Because matrix_dev->mdevs_lock is acquired inside
the loop, it is dropped before the list_next_entry() macro step evaluates
the next pointer for the iteration.

Since vfio_ap_mdev_probe() adds devices to this list while holding only
matrix_dev->mdevs_lock:

vfio_ap_mdev_probe() {
    ...
    mutex_lock(&matrix_dev->mdevs_lock);
    list_add(&matrix_mdev->node, &matrix_dev->mdev_list);
    mutex_unlock(&matrix_dev->mdevs_lock);
    ...
}

Could a concurrent bus scan and probe cause this traversal in
vfio_ap_on_scan_complete() to read a partially updated list pointer and
cause list corruption?

> +		/*
> +		 * The mdevs_lock must be held to access fields within matrix_mdev,
> +		 * and kvm->lock must be taken before mdevs_lock to satisfy the lock
> +		 * ordering requirement and prevent a lockdep splat.
> +		 */
> +		if (matrix_mdev->kvm)
> +			mutex_lock(&matrix_mdev->kvm->lock);
> +		mutex_lock(&matrix_dev->mdevs_lock);
> +
>  		if (bitmap_empty(matrix_mdev->apm_add, AP_DEVICES) &&
>  		    bitmap_empty(matrix_mdev->aqm_add, AP_DOMAINS) &&
>  		    bitmap_empty(matrix_mdev->adm_add, AP_DOMAINS))
[ ... ]

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

[PATCH v5 3/9] s390/vfio-ap: Fix missing lock required to access list of ap_matrix_mdev objects

[Anthony Krowiak]

In order to traverse or add/remove ap_matrix_mdev objects in the
matrix_dev->mdev_list, the matrix_dev->guests_lock mutex must be held.
There are two functions that access the list without holding the mutex:

vfio_ap_mdev_probe function
~~~~~~~~~~~~~~~~~~~~~~~~~~~
The vfio_ap_mdev_probe function uses the matrix_dev->mdevs_lock
mutex to guard the add of a newly created ap_matrix_mdev object to the
matrix_dev->mdev_list. This mutex does not protect list access; its purpose
is to guard against concurrent access to fields contained in an
ap_matrix_mdev object. This could lead to kernel memory corruption or
use-after-free if another mdev is created or removed concurrently.

The adding of an ap_matrix_mdev object to matrix_dev->mdev_list
is now guarded by the matrix_dev->guests_lock which is the correct
way to protect against concurrent mdev_list access.

Also removed the following two lines of code because the matrix_mdev is
allocated via vfio_alloc_device macro which uses kzalloc, so req_trigger
and cfg_chg_trigger are already zero-initialised when the struct is
allocated before the call to vfio_register_emulated_iommu_dev. This
prevents a window whereby these triggers are set to NULL after
the device is exposed to userspace.

matrix_mdev->req_trigger = NULL;
matrix_mdev->cfg_chg_trigger = NULL;

vfio_ap_mdev_for_queue function
~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~
The status_show function that supports display of the status attribute of
the devices in /sys/bus/ap/devices calls the vfio_ap_mdev_for_queue
function which iterates the matrix_dev->mdev_list to find the object
representing the queue device whose status is to be displayed. In order to
traverse this list, the matrix_dev->guests_lock mutex must be held.

To fix this, the guests_lock mutex is taken prior to taking the
matrix_dev->mdevs_lock mutex in the status_show function. It is taken
there rather than the vfio_ap_mdev_for_queue function - where it is
needed - because it must be taken prior to the mdevs_lock mutex in order to
adhere to the proper locking order and prevent a lockdep splat; also
because the mdevs_lock is needed there to access fields within
the matrix_mdev object in that function.

See the vfio-ap-locking.rst in the linux kernel tree.

Fixes: 2c1ee8983aa3 ("s390/vfio-ap: prepare for dynamic update of guest's APCB on queue probe/remove")
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 | 27 +++++++++++++++++++++++----
 1 file changed, 23 insertions(+), 4 deletions(-)

diff --git a/drivers/s390/crypto/vfio_ap_ops.c b/drivers/s390/crypto/vfio_ap_ops.c
index c6bee69cc22f..f2d662e388bd 100644
--- a/drivers/s390/crypto/vfio_ap_ops.c
+++ b/drivers/s390/crypto/vfio_ap_ops.c
@@ -800,12 +800,17 @@ static int vfio_ap_mdev_probe(struct mdev_device *mdev)
 	ret = vfio_register_emulated_iommu_dev(&matrix_mdev->vdev);
 	if (ret)
 		goto err_put_vdev;
-	matrix_mdev->req_trigger = NULL;
-	matrix_mdev->cfg_chg_trigger = NULL;
+
+	/*
+	 * Take the matrix_dev->guests_lock mutex before adding the matrix_mdev
+	 * to the mdev_list. All functions that traverse the list must also hold
+	 * this lock to guard against additions to or removals from the list
+	 * while it is being traversed.
+	 */
+	mutex_lock(&matrix_dev->guests_lock);
 	dev_set_drvdata(&mdev->dev, matrix_mdev);
-	mutex_lock(&matrix_dev->mdevs_lock);
 	list_add(&matrix_mdev->node, &matrix_dev->mdev_list);
-	mutex_unlock(&matrix_dev->mdevs_lock);
+	mutex_unlock(&matrix_dev->guests_lock);
 	return 0;
 
 err_put_vdev:
@@ -2297,6 +2302,8 @@ static struct ap_matrix_mdev *vfio_ap_mdev_for_queue(struct vfio_ap_queue *q)
 	unsigned long apid = AP_QID_CARD(q->apqn);
 	unsigned long apqi = AP_QID_QUEUE(q->apqn);
 
+	lockdep_assert_held(&matrix_dev->guests_lock);
+
 	list_for_each_entry(matrix_mdev, &matrix_dev->mdev_list, node) {
 		if (test_bit_inv(apid, matrix_mdev->matrix.apm) &&
 		    test_bit_inv(apqi, matrix_mdev->matrix.aqm))
@@ -2316,6 +2323,7 @@ static ssize_t status_show(struct device *dev,
 	struct ap_matrix_mdev *matrix_mdev;
 	struct ap_device *apdev = to_ap_dev(dev);
 
+	mutex_lock(&matrix_dev->guests_lock);
 	mutex_lock(&matrix_dev->mdevs_lock);
 	q = dev_get_drvdata(&apdev->device);
 	matrix_mdev = vfio_ap_mdev_for_queue(q);
@@ -2343,6 +2351,7 @@ static ssize_t status_show(struct device *dev,
 	}
 
 	mutex_unlock(&matrix_dev->mdevs_lock);
+	mutex_unlock(&matrix_dev->guests_lock);
 
 	return nchars;
 }
@@ -2761,6 +2770,12 @@ static void vfio_ap_mdev_cfg_add(unsigned long *apm_add, unsigned long *aqm_add,
 
 	vfio_ap_filter_apid_by_qtype(apm_add, aqm_add);
 
+	/*
+	 * It is safe to traverse this list here because the
+	 * required guard - matrix_dev->guests_lock - is taken in the
+	 * vfio_ap_on_cfg_changed function prior to this function getting
+	 * called.
+	 */
 	list_for_each_entry(matrix_mdev, &matrix_dev->mdev_list, node) {
 		bitmap_and(matrix_mdev->apm_add,
 			   matrix_mdev->matrix.apm, apm_add, AP_DEVICES);
@@ -2820,6 +2835,10 @@ void vfio_ap_on_cfg_changed(struct ap_config_info *cur_cfg_info,
 	if (!cur_cfg_info || !prev_cfg_info)
 		return;
 
+	/*
+	 * Take the guests_lock mutex here to guard access to the
+	 * matrix_dev->mdev_list in the two functions called below.
+	 */
 	mutex_lock(&matrix_dev->guests_lock);
 
 	vfio_ap_mdev_on_cfg_remove(cur_cfg_info, prev_cfg_info);
-- 
2.53.0

Re: [PATCH v5 3/9] s390/vfio-ap: Fix missing lock required to access list of ap_matrix_mdev objects

[sashiko-bot]

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

Pre-existing issues:
- [Critical] Registering the emulated IOMMU device before adding it to the mdev list allows a race condition where multiple mdevs can be attached to the same KVM instance, bypassing the strict 1-to-1 isolation check.
- [High] Eventfd context references (`req_trigger` and `cfg_chg_trigger`) are leaked if the VFIO device is closed or removed without explicitly unregistering the IRQs.
--

--- Patch [3]: [PATCH v5 3/9] s390/vfio-ap: Fix missing lock required to access list of ap_matrix_mdev objects ---
commit 225a264b9dad5df5a833e518a4dd594cafd35d03
Author: Anthony Krowiak <akrowiak@linux.ibm.com>

s390/vfio-ap: Fix missing lock required to access list of ap_matrix_mdev objects

This commit fixes a locking issue where the mdev_list was accessed without
holding the matrix_dev->guests_lock mutex. It ensures the lock is properly
acquired during device probe and status show operations. Unnecessary NULL
initializations for triggers are also removed.

> diff --git a/drivers/s390/crypto/vfio_ap_ops.c b/drivers/s390/crypto/vfio_ap_ops.c
> index c6bee69cc22f6..f2d662e388bdc 100644
> --- a/drivers/s390/crypto/vfio_ap_ops.c
> +++ b/drivers/s390/crypto/vfio_ap_ops.c
> @@ -800,12 +800,17 @@ static int vfio_ap_mdev_probe(struct mdev_device *mdev)
>  	ret = vfio_register_emulated_iommu_dev(&matrix_mdev->vdev);
>  	if (ret)
>  		goto err_put_vdev;

[Severity: Critical]
This is a pre-existing issue, but does registering the emulated IOMMU
device here expose it to userspace before it is added to the mdev list?

If userspace concurrently opens the device and calls vfio_ap_mdev_set_kvm(),
that function traverses the mdev list to check for duplicate KVM references.
Since the device is not in the list yet, this check can succeed. Could this
allow multiple mdevs to attach to the same KVM instance and bypass the
isolation check?

> -	matrix_mdev->req_trigger = NULL;
> -	matrix_mdev->cfg_chg_trigger = NULL;

[Severity: High]
This is a pre-existing issue, but does this code leak the eventfd contexts
if userspace sets them and then closes the device without explicitly
unregistering the IRQs?

When userspace sets triggers via VFIO_DEVICE_SET_IRQS, eventfd_ctx_fdget()
takes a reference. If the device fd is later closed, the cleanup sequence
happens:

vfio_ap_mdev_close_device()
vfio_ap_mdev_remove()
   vfio_put_device(&matrix_mdev->vdev);

Neither function appears to release the req_trigger or cfg_chg_trigger
references. Can this lead to an indefinite leak of the eventfd_ctx memory
since there is no release callback to clean them up when the matrix_mdev
is freed?

> +
> +	/*
> +	 * Take the matrix_dev->guests_lock mutex before adding the matrix_mdev
> +	 * to the mdev_list. All functions that traverse the list must also hold
> +	 * this lock to guard against additions to or removals from the list
> +	 * while it is being traversed.
> +	 */
> +	mutex_lock(&matrix_dev->guests_lock);
>  	dev_set_drvdata(&mdev->dev, matrix_mdev);
> -	mutex_lock(&matrix_dev->mdevs_lock);
>  	list_add(&matrix_mdev->node, &matrix_dev->mdev_list);
> -	mutex_unlock(&matrix_dev->mdevs_lock);
> +	mutex_unlock(&matrix_dev->guests_lock);
>  	return 0;

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

[PATCH v5 4/9] s390/vfio-ap: Fix required lock not held during update of ap_matrix_mdev object

[Anthony Krowiak]

In the vfio_ap_mdev_cfg_add function, the apm_add, aqm_add and adm_add
fields of an ap_matrix_mdev object fields are modified while not holding
the matrix_dev->mdevs_lock. This lock must be held while making these
to guard against a race condition with another caller that may be
concurrently modifying these fields or any of the fields in the
matrix_mdev->matrix.

Fixes: eeb386aeb5b7c ("s390/vfio-ap: handle config changed and scan complete notification")
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 | 8 ++++++++
 1 file changed, 8 insertions(+)

diff --git a/drivers/s390/crypto/vfio_ap_ops.c b/drivers/s390/crypto/vfio_ap_ops.c
index f2d662e388bd..21c502598f8c 100644
--- a/drivers/s390/crypto/vfio_ap_ops.c
+++ b/drivers/s390/crypto/vfio_ap_ops.c
@@ -2777,12 +2777,20 @@ static void vfio_ap_mdev_cfg_add(unsigned long *apm_add, unsigned long *aqm_add,
 	 * called.
 	 */
 	list_for_each_entry(matrix_mdev, &matrix_dev->mdev_list, node) {
+		/*
+		 * The mdevs_lock must be held in order to access fields
+		 * within matrix_mdev
+		 */
+		mutex_lock(&matrix_dev->mdevs_lock);
+
 		bitmap_and(matrix_mdev->apm_add,
 			   matrix_mdev->matrix.apm, apm_add, AP_DEVICES);
 		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);
+
+		mutex_unlock(&matrix_dev->mdevs_lock);
 	}
 }
 
-- 
2.53.0

Re: [PATCH v5 4/9] s390/vfio-ap: Fix required lock not held during update of ap_matrix_mdev object

[sashiko-bot]

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

Pre-existing issues:
- [High] Hot-plug add state is overwritten instead of accumulated, potentially dropping hot-plug events.
- [High] Pinned page and GISC registration leak in vfio_ap_irq_enable() when ap_aqic() returns a default error code.
--

--- Patch [4]: [PATCH v5 4/9] s390/vfio-ap: Fix required lock not held during update of ap_matrix_mdev object ---
commit 65680656f25b3ea60807c838ba28f8d65cca5a45
Author: Anthony Krowiak <akrowiak@linux.ibm.com>

s390/vfio-ap: Fix required lock not held during update of ap_matrix_mdev object

This commit adds the missing matrix_dev->mdevs_lock in vfio_ap_mdev_cfg_add()
to guard against race conditions when modifying the ap_matrix_mdev object's
fields.

> diff --git a/drivers/s390/crypto/vfio_ap_ops.c b/drivers/s390/crypto/vfio_ap_ops.c
> index f2d662e388bdc..21c502598f8ca 100644
> --- a/drivers/s390/crypto/vfio_ap_ops.c
> +++ b/drivers/s390/crypto/vfio_ap_ops.c
> @@ -2777,12 +2777,20 @@ static void vfio_ap_mdev_cfg_add(unsigned long *apm_add, unsigned long *aqm_add,
>  	 * called.
>  	 */
>  	list_for_each_entry(matrix_mdev, &matrix_dev->mdev_list, node) {
> +		/*
> +		 * The mdevs_lock must be held in order to access fields
> +		 * within matrix_mdev
> +		 */
> +		mutex_lock(&matrix_dev->mdevs_lock);
> +
>  		bitmap_and(matrix_mdev->apm_add,
>  			   matrix_mdev->matrix.apm, apm_add, AP_DEVICES);
>  		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);

[Severity: High]
This isn't a bug introduced by this patch, but does this overwrite the
hot-plug addition state instead of accumulating it?

If multiple AP configuration change events occur before the AP bus scan
completes, it appears that vfio_ap_mdev_cfg_add() unconditionally overwrites
the deferred hot-plug bitmaps with the delta of the most recent event.

Should this logic use a temporary intersection followed by bitmap_or() to
accumulate the pending additions so that hot-plug events aren't dropped?

> +
> +		mutex_unlock(&matrix_dev->mdevs_lock);
>  	}
>  }

While reviewing the file, I noticed a pre-existing issue in
vfio_ap_irq_enable():

> drivers/s390/crypto/vfio_ap_ops.c:vfio_ap_irq_enable() {
> 	...
> 	default:
> 		pr_warn("%s: apqn %04x: response: %02x\n", __func__, q->apqn,
> 			status.response_code);
> 		vfio_ap_irq_disable(q);
> 		break;
> 	}
> 	...
> }

[Severity: High]
This is a pre-existing issue, but does this default error path leak the
newly pinned nib and registered nisc?

When ap_aqic() returns an unexpected error falling into the default case,
vfio_ap_irq_disable() is called. However, it seems vfio_ap_irq_disable()
only cleans up the previously successfully saved configuration, bypassing
the newly pinned nib and isc held in local variables.

Can a malicious KVM guest repeatedly issue the PQAP(AQIC) instruction with
configurations that trigger this error path, causing unbounded memory pinning
on the host?

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

Re: [PATCH v5 4/9] s390/vfio-ap: Fix required lock not held during update of ap_matrix_mdev object

[Anthony Krowiak]

On 8/12/26 4:18 PM, sashiko-bot@kernel.org wrote:
> Thank you for your contribution! Sashiko AI review found 2 potential issue(s) to consider:
>
> Pre-existing issues:
> - [High] Hot-plug add state is overwritten instead of accumulated, potentially dropping hot-plug events.
> - [High] Pinned page and GISC registration leak in vfio_ap_irq_enable() when ap_aqic() returns a default error code.
> --
>
> --- Patch [4]: [PATCH v5 4/9] s390/vfio-ap: Fix required lock not held during update of ap_matrix_mdev object ---
> commit 65680656f25b3ea60807c838ba28f8d65cca5a45
> Author: Anthony Krowiak <akrowiak@linux.ibm.com>
>
> s390/vfio-ap: Fix required lock not held during update of ap_matrix_mdev object
>
> This commit adds the missing matrix_dev->mdevs_lock in vfio_ap_mdev_cfg_add()
> to guard against race conditions when modifying the ap_matrix_mdev object's
> fields.
>
>> diff --git a/drivers/s390/crypto/vfio_ap_ops.c b/drivers/s390/crypto/vfio_ap_ops.c
>> index f2d662e388bdc..21c502598f8ca 100644
>> --- a/drivers/s390/crypto/vfio_ap_ops.c
>> +++ b/drivers/s390/crypto/vfio_ap_ops.c
>> @@ -2777,12 +2777,20 @@ static void vfio_ap_mdev_cfg_add(unsigned long *apm_add, unsigned long *aqm_add,
>>   	 * called.
>>   	 */
>>   	list_for_each_entry(matrix_mdev, &matrix_dev->mdev_list, node) {
>> +		/*
>> +		 * The mdevs_lock must be held in order to access fields
>> +		 * within matrix_mdev
>> +		 */
>> +		mutex_lock(&matrix_dev->mdevs_lock);
>> +
>>   		bitmap_and(matrix_mdev->apm_add,
>>   			   matrix_mdev->matrix.apm, apm_add, AP_DEVICES);
>>   		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);
> [Severity: High]
> This isn't a bug introduced by this patch, but does this overwrite the
> hot-plug addition state instead of accumulating it?
>
> If multiple AP configuration change events occur before the AP bus scan
> completes, it appears that vfio_ap_mdev_cfg_add() unconditionally overwrites
> the deferred hot-plug bitmaps with the delta of the most recent event.
>
> Should this logic use a temporary intersection followed by bitmap_or() to
> accumulate the pending additions so that hot-plug events aren't dropped?

Looking at ap_scan_bus(), the entire execution is strictly sequential and
single-threaded under ap_scan_bus_mutex:

1. ap_get_configuration() is called once — a single PQAP(QCI)
    captures the snapshot of the configuration delta for the entire scan.

2. If config_changed, notify_config_changed() is called once — this
     fires vfio_ap_on_cfg_changed -> vfio_ap_mdev_cfg_add exactly once 
per scan.

3. The adapter loop runs.

4. If config_changed, notify_scan_complete() is called once — this
     fires vfio_ap_on_scan_complete exactly once per scan.

Both notify_config_changed and notify_scan_complete are also guarded by
ap_scan_bus_mutex which ensures the entire callback pair is an
atomic unit from the perspective of any concurrent scan attempt.
The mutex_trylock(ap_scan_bus_mutex) is used, so a concurrent scan
request is simply dropped if one is already in progress, not queued 
behind it.

So the scenario the that two on_cfg_changed firings accumulating
in the bitmaps before on_scan_complete clears them is structurally 
impossible.
Each scan atomically fires on_cfg_changed (at most once) and 
on_scan_complete
(at most once) as an inseparable pair, with no mechanism for a second 
scan to
interleave. The comment is not valid.

>
>> +
>> +		mutex_unlock(&matrix_dev->mdevs_lock);
>>   	}
>>   }
> While reviewing the file, I noticed a pre-existing issue in
> vfio_ap_irq_enable():
>
>> drivers/s390/crypto/vfio_ap_ops.c:vfio_ap_irq_enable() {
>> 	...
>> 	default:
>> 		pr_warn("%s: apqn %04x: response: %02x\n", __func__, q->apqn,
>> 			status.response_code);
>> 		vfio_ap_irq_disable(q);
>> 		break;
>> 	}
>> 	...
>> }
> [Severity: High]
> This is a pre-existing issue, but does this default error path leak the
> newly pinned nib and registered nisc?
>
> When ap_aqic() returns an unexpected error falling into the default case,
> vfio_ap_irq_disable() is called. However, it seems vfio_ap_irq_disable()
> only cleans up the previously successfully saved configuration, bypassing
> the newly pinned nib and isc held in local variables.
>
> Can a malicious KVM guest repeatedly issue the PQAP(AQIC) instruction with
> configurations that trigger this error path, causing unbounded memory pinning
> on the host?

This one is a real pre-existing bug. By the time the default case is
reached:

1. The nib has been pinned
2. The nisc has been registered

Neither has been saved to q->saved_iova / q->saved_isc yet, so when
vfio_ap_irq_disable(q) calls vfio_ap_free_aqic_resources(q), it releases
whatever is in q->saved_iova and q->saved_isc; the old resources from a
prior call. The newly pinned nib page and newly registered nisc are never
freed.

A fix patch is forthcoming.

>

[PATCH v5 5/9] s390/vfio-ap: Fix control domain removal in vfio_ap_mdev_cfg_remove

[Anthony Krowiak]

The vfio_ap_config_remove function uses the bitmap_andnot function to clear
bits from the matrix_mdev->matrix.adm bitmap (specifies the control domains
assigned to the mdev). This prevents the explicitly unplugged control
domains from being removed the KVM guest. The bitmap_and function is used
instead.

Fixes: eeb386aeb5b7c ("s390/vfio-ap: handle config changed and scan complete notification")
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 | 6 +++---
 1 file changed, 3 insertions(+), 3 deletions(-)

diff --git a/drivers/s390/crypto/vfio_ap_ops.c b/drivers/s390/crypto/vfio_ap_ops.c
index 21c502598f8c..0f3537aadea8 100644
--- a/drivers/s390/crypto/vfio_ap_ops.c
+++ b/drivers/s390/crypto/vfio_ap_ops.c
@@ -2636,9 +2636,9 @@ static void vfio_ap_mdev_cfg_remove(unsigned long *ap_remove,
 		do_remove |= bitmap_and(aqrem, aq_remove,
 					  matrix_mdev->matrix.aqm,
 					  AP_DOMAINS);
-		do_remove |= bitmap_andnot(cdrem, cd_remove,
-					     matrix_mdev->matrix.adm,
-					     AP_DOMAINS);
+		do_remove |= bitmap_and(cdrem, cd_remove,
+					matrix_mdev->matrix.adm,
+					AP_DOMAINS);
 
 		if (do_remove)
 			vfio_ap_mdev_hot_unplug_cfg(matrix_mdev, aprem, aqrem,
-- 
2.53.0

Re: [PATCH v5 5/9] s390/vfio-ap: Fix control domain removal in vfio_ap_mdev_cfg_remove

[sashiko-bot]

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

Pre-existing issues:
- [High] vfio_ap_mdev_hot_unplug_cfg incorrectly uses the return value of bitmap_andnot to track configuration changes, skipping KVM guest updates when the last AP resource is unplugged.
--

--- Patch [5]: [PATCH v5 5/9] s390/vfio-ap: Fix control domain removal in vfio_ap_mdev_cfg_remove ---
commit 2e94c01dff5c7206d21da99863677a285a17a31f
Author: Anthony Krowiak <akrowiak@linux.ibm.com>
s390/vfio-ap: Fix control domain removal in vfio_ap_mdev_cfg_remove

This commit fixes the removal of control domains in the vfio_ap_mdev_cfg_remove
function. It correctly uses bitmap_and instead of bitmap_andnot to clear the
assigned control domains, allowing unplugged control domains to be properly
removed from the KVM guest.

> diff --git a/drivers/s390/crypto/vfio_ap_ops.c b/drivers/s390/crypto/vfio_ap_ops.c
> index 21c502598f8ca..0f3537aadea87 100644
> --- a/drivers/s390/crypto/vfio_ap_ops.c
> +++ b/drivers/s390/crypto/vfio_ap_ops.c
> @@ -2636,9 +2636,9 @@ static void vfio_ap_mdev_cfg_remove(unsigned long *ap_remove,
>  		do_remove |= bitmap_and(aqrem, aq_remove,
>  					  matrix_mdev->matrix.aqm,
>  					  AP_DOMAINS);
> -		do_remove |= bitmap_andnot(cdrem, cd_remove,
> -					     matrix_mdev->matrix.adm,
> -					     AP_DOMAINS);
> +		do_remove |= bitmap_and(cdrem, cd_remove,
> +					matrix_mdev->matrix.adm,
> +					AP_DOMAINS);
>  
>  		if (do_remove)
>  			vfio_ap_mdev_hot_unplug_cfg(matrix_mdev, aprem, aqrem,

[Severity: High]
This is a pre-existing issue, but does vfio_ap_mdev_hot_unplug_cfg() correctly
handle configuration updates when the last AP resource is unplugged?

Looking at vfio_ap_mdev_hot_unplug_cfg():

drivers/s390/crypto/vfio_ap_ops.c:vfio_ap_mdev_hot_unplug_cfg() {
        int do_hotplug = 0;

        if (!bitmap_empty(aprem, AP_DEVICES)) {
                do_hotplug |= bitmap_andnot(matrix_mdev->shadow_apcb.apm,
                                            matrix_mdev->shadow_apcb.apm,
                                            aprem, AP_DEVICES);
        }

[ ... ]

        if (do_hotplug)
                vfio_ap_mdev_update_guest_apcb(matrix_mdev);
}

The bitmap_andnot() function returns true only if the resulting destination
bitmap is non-empty. If the last device or domain is removed, the resulting
bitmap becomes empty, meaning bitmap_andnot() returns false.

Consequently, do_hotplug evaluates to false, and the guest update function
vfio_ap_mdev_update_guest_apcb() is never called.

Could this skip updating the KVM guest, leaving it with access to AP hardware
that was hot-unplugged from the host configuration?

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

[PATCH v5 6/9] s390/vfio-ap: fix potential use of uninitialized apm_filtered bitmap

[Anthony Krowiak]

The DECLARE_BITMAP(apm_filtered, AP_DEVICES) macro allocates the bitmap
on the stack without zero-initializing it.

In vfio_ap_mdev_hot_plug_cfg(), the vfio_ap_mdev_filter_matrix() function
is only called to initialize and populate apm_filtered if either
filter_adapters or filter_domains is true. If the hot plug configuration
change only adds control domains (meaning filter_cdoms is true, but
filter_adapters and filter_domains are both false),
vfio_ap_mdev_filter_matrix() is bypassed.

Consequently, apm_filtered is passed to reset_queues_for_apids() with
uninitialized stack garbage. This can cause reset_queues_for_apids() to
interpret arbitrary stack garbage bits as valid APIDs to reset, potentially
performing unintended guest hardware queue resets.

Fix this by zero-initializing the apm_filtered bitmap at the beginning of
vfio_ap_mdev_hot_plug_cfg() using bitmap_zero().

Fixes: eeb386aeb5b7c ("s390/vfio-ap: handle config changed and scan complete notification")
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 | 9 +++++++++
 1 file changed, 9 insertions(+)

diff --git a/drivers/s390/crypto/vfio_ap_ops.c b/drivers/s390/crypto/vfio_ap_ops.c
index 0f3537aadea8..d667ad4bbf70 100644
--- a/drivers/s390/crypto/vfio_ap_ops.c
+++ b/drivers/s390/crypto/vfio_ap_ops.c
@@ -2861,6 +2861,15 @@ static void vfio_ap_mdev_hot_plug_cfg(struct ap_matrix_mdev *matrix_mdev)
 	DECLARE_BITMAP(apm_filtered, AP_DEVICES);
 	bool filter_domains, filter_adapters, filter_cdoms, do_hotplug = false;
 
+	/*
+	 * Zero out the apm_filtered bitmap in case there are no adapters or
+	 * domains to be added, but only control domains. In that case,
+	 * vfio_ap_mdev_filter_matrix() - which initializes apm_filtered - will
+	 * not get called and the reset_queues_for_apids will crash because it
+	 * will access an uninitialized bitmap.
+	 */
+	bitmap_zero(apm_filtered, AP_DEVICES);
+
 	filter_adapters = bitmap_intersects(matrix_mdev->matrix.apm,
 					    matrix_mdev->apm_add, AP_DEVICES);
 	filter_domains = bitmap_intersects(matrix_mdev->matrix.aqm,
-- 
2.53.0

Re: [PATCH v5 6/9] s390/vfio-ap: fix potential use of uninitialized apm_filtered bitmap

[sashiko-bot]

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

Pre-existing issues:
- [High] Leaked pinned NIB memory and KVM GISC registration in `vfio_ap_irq_enable()` error path.
- [High] Leaked `req_trigger` and `cfg_chg_trigger` eventfd contexts on VFIO device close/remove.
--

--- Patch [6]: [PATCH v5 6/9] s390/vfio-ap: fix potential use of uninitialized apm_filtered bitmap ---
commit 6362e7fffee6cfc724530d3fd54b669440496a88
Author: Anthony Krowiak <akrowiak@linux.ibm.com>

s390/vfio-ap: fix potential use of uninitialized apm_filtered bitmap

The DECLARE_BITMAP(apm_filtered, AP_DEVICES) macro allocates the bitmap
on the stack without zero-initializing it. This can cause unintended
guest hardware queue resets if vfio_ap_mdev_filter_matrix() is bypassed.
This patch zero-initializes the apm_filtered bitmap at the beginning of
vfio_ap_mdev_hot_plug_cfg() using bitmap_zero().

> diff --git a/drivers/s390/crypto/vfio_ap_ops.c b/drivers/s390/crypto/vfio_ap_ops.c
> index 0f3537aadea87..d667ad4bbf705 100644
> --- a/drivers/s390/crypto/vfio_ap_ops.c
> +++ b/drivers/s390/crypto/vfio_ap_ops.c
> @@ -2861,6 +2861,15 @@ static void vfio_ap_mdev_hot_plug_cfg(struct ap_matrix_mdev *matrix_mdev)
>  	DECLARE_BITMAP(apm_filtered, AP_DEVICES);
>  	bool filter_domains, filter_adapters, filter_cdoms, do_hotplug = false;
>  
> +	/*
> +	 * Zero out the apm_filtered bitmap in case there are no adapters or
> +	 * domains to be added, but only control domains. In that case,
> +	 * vfio_ap_mdev_filter_matrix() - which initializes apm_filtered - will
> +	 * not get called and the reset_queues_for_apids will crash because it
> +	 * will access an uninitialized bitmap.
> +	 */
> +	bitmap_zero(apm_filtered, AP_DEVICES);
> +
>  	filter_adapters = bitmap_intersects(matrix_mdev->matrix.apm,
>  					    matrix_mdev->apm_add, AP_DEVICES);
>  	filter_domains = bitmap_intersects(matrix_mdev->matrix.aqm,

[ ... ]

> 	case AP_RESPONSE_OTHERWISE_CHANGED:
> 		/* 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;
> 	default:
> 		pr_warn("%s: apqn %04x: response: %02x\n", __func__, q->apqn,
> 			status.response_code);
> 		vfio_ap_irq_disable(q);
> 		break;
> 	}

[Severity: High]
This is a pre-existing issue, but does this default error path in
vfio_ap_irq_enable() leak the newly allocated NIB pages and KVM GISC
registration?

If a user-space process configures device IRQs and triggers
vfio_ap_irq_enable(), the function successfully pins the new NIB and
registers the GISC. If ap_aqic() fails with an unexpected status code, it
falls through to this default case.

Since vfio_ap_irq_disable(q) only cleans up previously saved resources in
q->saved_iova and q->saved_isc, it appears the newly pinned nib and isc
local variables are left abandoned.

[ ... ]

> 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);
> }

[Severity: High]
This is a pre-existing issue, but it looks like this teardown path might
leak the req_trigger and cfg_chg_trigger eventfd contexts.

When a user-space process sets up IRQs, eventfd_ctx_fdget() takes a
reference on the eventfds. If the process crashes or closes the VFIO device
file without explicitly clearing the IRQs, shouldn't
vfio_ap_mdev_close_device() drop these references with eventfd_ctx_put()?

[ ... ]

> static void vfio_ap_mdev_remove(struct mdev_device *mdev)
> {
> 	struct ap_matrix_mdev *matrix_mdev = dev_get_drvdata(&mdev->dev);
> 
> 	vfio_unregister_group_dev(&matrix_mdev->vdev);
> 
> 	mutex_lock(&matrix_dev->guests_lock);
> 	mutex_lock(&matrix_dev->mdevs_lock);
> 	vfio_ap_mdev_reset_queues(matrix_mdev);
> 	vfio_ap_mdev_unlink_fr_queues(matrix_mdev);
> 	list_del(&matrix_mdev->node);
> 	mutex_unlock(&matrix_dev->mdevs_lock);
> 	mutex_unlock(&matrix_dev->guests_lock);
> 	vfio_put_device(&matrix_mdev->vdev);
> }

[Severity: High]
This is a pre-existing issue, but similar to the above, is it possible for
the mdev teardown path in vfio_ap_mdev_remove() to free the device
structure without cleaning up the eventfd context references?

Could repeatedly opening and closing the device exhaust system file
descriptors and kernel memory due to these missing eventfd_ctx_put()
calls?

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

[PATCH v5 7/9] s390/vfio-ap: Fix hot-unplug skipped when last AP adapter or domain removed

[Anthony Krowiak]

The vfio_ap_mdev_hot_unplug_cfg() function uses the return value of
bitmap_andnot() to determine whether the guest APCB needs to be updated.
However, bitmap_andnot() returns false when the resulting destination
bitmap is empty. This means that if the only adapter, domain or control
domain assigned to an mdev is removed from the host's AP configuration,
the bit is correctly cleared from the shadow APCB, but bitmap_andnot()
returns false because the result is an empty bitmap. Consequently,
do_hotplug remains 0 and vfio_ap_mdev_update_guest_apcb() is never called,
leaving the KVM guest with stale hardware access to the unplugged AP
devices.

Fix this by replacing the bitmap_andnot() return value check with
bitmap_intersects() to determine whether the shadow APCB actually
overlaps with the removal mask. If there is an intersection, call
bitmap_andnot() solely for its side effect of clearing the bits, then
unconditionally set do_hotplug to trigger the guest APCB update.

Fixes: eeb386aeb5b7c ("s390/vfio-ap: handle config changed and scan complete notification")
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 | 30 +++++++++++++++++-------------
 1 file changed, 17 insertions(+), 13 deletions(-)

diff --git a/drivers/s390/crypto/vfio_ap_ops.c b/drivers/s390/crypto/vfio_ap_ops.c
index d667ad4bbf70..16779cfc64e8 100644
--- a/drivers/s390/crypto/vfio_ap_ops.c
+++ b/drivers/s390/crypto/vfio_ap_ops.c
@@ -2568,24 +2568,28 @@ static void vfio_ap_mdev_hot_unplug_cfg(struct ap_matrix_mdev *matrix_mdev,
 					unsigned long *aqrem,
 					unsigned long *cdrem)
 {
-	int do_hotplug = 0;
+	bool do_hotplug = false;
 
-	if (!bitmap_empty(aprem, AP_DEVICES)) {
-		do_hotplug |= bitmap_andnot(matrix_mdev->shadow_apcb.apm,
-					    matrix_mdev->shadow_apcb.apm,
-					    aprem, AP_DEVICES);
+	if (bitmap_intersects(matrix_mdev->shadow_apcb.apm, aprem, AP_DEVICES)) {
+		bitmap_andnot(matrix_mdev->shadow_apcb.apm,
+			      matrix_mdev->shadow_apcb.apm,
+			      aprem, AP_DEVICES);
+		do_hotplug = true;
 	}
 
-	if (!bitmap_empty(aqrem, AP_DOMAINS)) {
-		do_hotplug |= bitmap_andnot(matrix_mdev->shadow_apcb.aqm,
-					    matrix_mdev->shadow_apcb.aqm,
-					    aqrem, AP_DEVICES);
+	if (bitmap_intersects(matrix_mdev->shadow_apcb.aqm, aqrem, AP_DOMAINS)) {
+		bitmap_andnot(matrix_mdev->shadow_apcb.aqm,
+			      matrix_mdev->shadow_apcb.aqm,
+			      aqrem, AP_DOMAINS);
+		do_hotplug = true;
 	}
 
-	if (!bitmap_empty(cdrem, AP_DOMAINS))
-		do_hotplug |= bitmap_andnot(matrix_mdev->shadow_apcb.adm,
-					    matrix_mdev->shadow_apcb.adm,
-					    cdrem, AP_DOMAINS);
+	if (bitmap_intersects(matrix_mdev->shadow_apcb.adm, cdrem, AP_DOMAINS)) {
+		bitmap_andnot(matrix_mdev->shadow_apcb.adm,
+			      matrix_mdev->shadow_apcb.adm,
+			      cdrem, AP_DOMAINS);
+		do_hotplug = true;
+	}
 
 	if (do_hotplug)
 		vfio_ap_mdev_update_guest_apcb(matrix_mdev);
-- 
2.53.0

Re: [PATCH v5 7/9] s390/vfio-ap: Fix hot-unplug skipped when last AP adapter or domain removed

[sashiko-bot]

> The vfio_ap_mdev_hot_unplug_cfg() function uses the return value of
> bitmap_andnot() to determine whether the guest APCB needs to be updated.
> However, bitmap_andnot() returns false when the resulting destination
> bitmap is empty. This means that if the only adapter, domain or control
> domain assigned to an mdev is removed from the host's AP configuration,
> the bit is correctly cleared from the shadow APCB, but bitmap_andnot()
> returns false because the result is an empty bitmap. Consequently,
> do_hotplug remains 0 and vfio_ap_mdev_update_guest_apcb() is never called,
> leaving the KVM guest with stale hardware access to the unplugged AP
> devices.
> 
> Fix this by replacing the bitmap_andnot() return value check with
> bitmap_intersects() to determine whether the shadow APCB actually
> overlaps with the removal mask. If there is an intersection, call
> bitmap_andnot() solely for its side effect of clearing the bits, then
> unconditionally set do_hotplug to trigger the guest APCB update.
> 
> Fixes: eeb386aeb5b7c ("s390/vfio-ap: handle config changed and scan complete notification")
> 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/20260812200240.818004-1-akrowiak@linux.ibm.com?part=7

Re: [PATCH v5 7/9] s390/vfio-ap: Fix hot-unplug skipped when last AP adapter or domain removed

[Halil Pasic]

On Wed, 12 Aug 2026 16:02:38 -0400
Anthony Krowiak <akrowiak@linux.ibm.com> wrote:

> @@ -2568,24 +2568,28 @@ static void vfio_ap_mdev_hot_unplug_cfg(struct ap_matrix_mdev *matrix_mdev,
>  					unsigned long *aqrem,
>  					unsigned long *cdrem)
>  {
> -	int do_hotplug = 0;
> +	bool do_hotplug = false;
>  

Nit: The name is IMHO somewhat misleading. If we are already touching
all uses of the variable and its type maybe we can use the opportunity
to change it's name as well.

Some ideas: update_apcb, changed, matrix_changed.

[..]
>  
>  	if (do_hotplug)
>  		vfio_ap_mdev_update_guest_apcb(matrix_mdev);
> -- 

[PATCH v5 8/9] s390/vfio-ap: Fix NULL deref in status_show() during queue probe

[Anthony Krowiak]

When vfio_ap_mdev_probe_queue() creates the sysfs attribute group,
the queue's driver data has not yet been set. A concurrent read of
the 'status' attribute can therefore call dev_get_drvdata() and
get NULL, which is then passed directly to
vfio_ap_mdev_for_queue() where q->apqn is unconditionally
dereferenced, causing a NULL pointer dereference.

Fix this by acquiring the update locks before calling
sysfs_create_group(). The status_show() function acquires
guests_lock before reading the driver data, so any concurrent
read will block until after dev_set_drvdata() has been called
and the update locks are released.

As a bonus, the APQN no longer needs to be read from the queue
struct after allocation — it can be read directly from apdev
before allocation and stored in a local variable, which is then
assigned to q->apqn once the allocation succeeds.

Fixes: 260f3ea141382 ("s390/vfio-ap: move probe and remove callbacks to vfio_ap_ops.c")
Cc: stable@vger.kernel.org
Signed-off-by: Anthony Krowiak <akrowiak@linux.ibm.com>
---
 drivers/s390/crypto/vfio_ap_ops.c | 33 +++++++++++++++++++++++++++----
 1 file changed, 29 insertions(+), 4 deletions(-)

diff --git a/drivers/s390/crypto/vfio_ap_ops.c b/drivers/s390/crypto/vfio_ap_ops.c
index 16779cfc64e8..1edd0b7a3cce 100644
--- a/drivers/s390/crypto/vfio_ap_ops.c
+++ b/drivers/s390/crypto/vfio_ap_ops.c
@@ -2326,6 +2326,23 @@ static ssize_t status_show(struct device *dev,
 	mutex_lock(&matrix_dev->guests_lock);
 	mutex_lock(&matrix_dev->mdevs_lock);
 	q = dev_get_drvdata(&apdev->device);
+
+	/*
+	 * Make sure the drvdata has been set before proceeding. There is a
+	 * possibility that the drvdata was not set if the vfio_ap_queue object
+	 * could not be allocated when the queue device was probed. In that case,
+	 * the locks used in vfio_ap_mdev_probe_queue() are released prior to
+	 * removing the sysfs status attribute to avoid a lockdep
+	 * splat. That opens a very small window where the status attribute is
+	 * still available without the vfio_ap_queue object having been
+	 * stored in the device drvdata. In that case, indicate the queue is not
+	 * assigned.
+	 */
+	if (!q) {
+		nchars = sysfs_emit(buf, "%s\n", AP_QUEUE_UNASSIGNED);
+		goto done;
+	}
+
 	matrix_mdev = vfio_ap_mdev_for_queue(q);
 
 	/* If the queue is assigned to the matrix mediated device, then
@@ -2350,6 +2367,7 @@ static ssize_t status_show(struct device *dev,
 		nchars = sysfs_emit(buf, "%s\n", AP_QUEUE_UNASSIGNED);
 	}
 
+done:
 	mutex_unlock(&matrix_dev->mdevs_lock);
 	mutex_unlock(&matrix_dev->guests_lock);
 
@@ -2424,14 +2442,17 @@ void vfio_ap_mdev_unregister(void)
 
 int vfio_ap_mdev_probe_queue(struct ap_device *apdev)
 {
-	int ret;
+	int ret, apqn;
 	struct vfio_ap_queue *q;
 	DECLARE_BITMAP(apm_filtered, AP_DEVICES);
 	struct ap_matrix_mdev *matrix_mdev;
 
+	apqn = to_ap_queue(&apdev->device)->qid;
+	matrix_mdev = get_update_locks_by_apqn(apqn);
+
 	ret = sysfs_create_group(&apdev->device.kobj, &vfio_queue_attr_group);
 	if (ret)
-		return ret;
+		goto err_release_locks;
 
 	q = kzalloc_obj(*q);
 	if (!q) {
@@ -2439,11 +2460,10 @@ int vfio_ap_mdev_probe_queue(struct ap_device *apdev)
 		goto err_remove_group;
 	}
 
-	q->apqn = to_ap_queue(&apdev->device)->qid;
+	q->apqn = apqn;
 	q->saved_isc = VFIO_AP_ISC_INVALID;
 	memset(&q->reset_status, 0, sizeof(q->reset_status));
 	INIT_WORK(&q->reset_work, apq_reset_check);
-	matrix_mdev = get_update_locks_by_apqn(q->apqn);
 
 	if (matrix_mdev) {
 		vfio_ap_mdev_link_queue(matrix_mdev, q);
@@ -2472,8 +2492,13 @@ int vfio_ap_mdev_probe_queue(struct ap_device *apdev)
 	return ret;
 
 err_remove_group:
+	release_update_locks_for_mdev(matrix_mdev);
 	sysfs_remove_group(&apdev->device.kobj, &vfio_queue_attr_group);
 	return ret;
+
+err_release_locks:
+	release_update_locks_for_mdev(matrix_mdev);
+	return ret;
 }
 
 void vfio_ap_mdev_remove_queue(struct ap_device *apdev)
-- 
2.53.0

Re: [PATCH v5 8/9] s390/vfio-ap: Fix NULL deref in status_show() during queue probe

[sashiko-bot]

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

Pre-existing issues:
- [High] Missing synchronization for asynchronous reset work during device removal leads to Use-After-Free.
--

--- Patch [8]: [PATCH v5 8/9] s390/vfio-ap: Fix NULL deref in status_show() during queue probe ---
commit 0bb399a6ce7f9a2aea3f2f595acfac62183158ba
Author: Anthony Krowiak <akrowiak@linux.ibm.com>

s390/vfio-ap: Fix NULL deref in status_show() during queue probe

This commit fixes a null pointer dereference in status_show() by checking
if the driver data has been set before proceeding. It also adjusts lock
acquisition during queue probe and simplifies the apqn assignment.

Cc: stable@vger.kernel.org

> diff --git a/drivers/s390/crypto/vfio_ap_ops.c b/drivers/s390/crypto/vfio_ap_ops.c
> index 16779cfc64e87..1edd0b7a3cce7 100644
> --- a/drivers/s390/crypto/vfio_ap_ops.c
> +++ b/drivers/s390/crypto/vfio_ap_ops.c

[ ... ]

> @@ -2472,8 +2492,13 @@ int vfio_ap_mdev_probe_queue(struct ap_device *apdev)
>  	return ret;
>  
>  err_remove_group:
> +	release_update_locks_for_mdev(matrix_mdev);
>  	sysfs_remove_group(&apdev->device.kobj, &vfio_queue_attr_group);
>  	return ret;
> +
> +err_release_locks:
> +	release_update_locks_for_mdev(matrix_mdev);
> +	return ret;
>  }
>  
>  void vfio_ap_mdev_remove_queue(struct ap_device *apdev)

[Severity: High]
This is a pre-existing issue, but looking at the rest of 
vfio_ap_mdev_remove_queue() below this signature, is there a potential
use-after-free if the queue is removed from the host config?

Further down in vfio_ap_mdev_remove_queue():

	if (test_bit_inv(apid, (unsigned long *)matrix_dev->info.apm) &&
	    test_bit_inv(apqi, (unsigned long *)matrix_dev->info.aqm)) {
		vfio_ap_mdev_reset_queue(q);
		flush_work(&q->reset_work);
	} else {
		vfio_ap_free_aqic_resources(q);
	}

done:
	if (matrix_mdev)
		vfio_ap_unlink_queue_fr_mdev(q);

	dev_set_drvdata(&apdev->device, NULL);
	kfree(q);

If an administrator hot-unplugs the ap adapter concurrently with a guest
triggering a reset, the queue is no longer in the host's configuration.

Because flush_work(&q->reset_work) is inside the conditional block, it is
skipped in this scenario. The function then proceeds directly to kfree(q).

Could the asynchronous reset worker thread (apq_reset_check) then execute
and dereference the freed q object, resulting in a use-after-free?

Should flush_work(&q->reset_work) be moved outside the condition to ensure
any pending reset work completes before the queue memory is freed?

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

Re: [PATCH v5 8/9] s390/vfio-ap: Fix NULL deref in status_show() during queue probe

[Matthew Rosato]

On 8/12/26 4:02 PM, Anthony Krowiak wrote:
> When vfio_ap_mdev_probe_queue() creates the sysfs attribute group,
> the queue's driver data has not yet been set. A concurrent read of
> the 'status' attribute can therefore call dev_get_drvdata() and
> get NULL, which is then passed directly to
> vfio_ap_mdev_for_queue() where q->apqn is unconditionally
> dereferenced, causing a NULL pointer dereference.
> 
> Fix this by acquiring the update locks before calling
> sysfs_create_group(). The status_show() function acquires
> guests_lock before reading the driver data, so any concurrent
> read will block until after dev_set_drvdata() has been called
> and the update locks are released.
> 
> As a bonus, the APQN no longer needs to be read from the queue
> struct after allocation — it can be read directly from apdev
> before allocation and stored in a local variable, which is then
> assigned to q->apqn once the allocation succeeds.
> 
> Fixes: 260f3ea141382 ("s390/vfio-ap: move probe and remove callbacks to vfio_ap_ops.c")
> Cc: stable@vger.kernel.org
> Signed-off-by: Anthony Krowiak <akrowiak@linux.ibm.com>

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

[PATCH v5 9/9] s390/vfio-ap: Fix memory leak when queue removed from host AP config

[Anthony Krowiak]

When an adapter or domain is removed from the host's AP configuration, the
AP bus invokes vfio_ap_on_cfg_changed() to notify the vfio_ap device
driver. For each ap_matrix_mdev object to which the adapter or domain
is assigned, vfio_ap_mdev_hot_unplug_cfg() is called and removes the
adapter or domain from the matrix_mdev->shadow_apcb (i.e., the guest's AP
configuration) and hot unplugs it if a guest is using it. The new host
AP configuration (sans adapter or domain) is then stored in
matrix_dev->info.

When the AP bus subsequently unbinds the physical queue devices associated
with the adapter or domain that has been removed, it invokes
vfio_ap_mdev_remove_queue() for each queue removed. At this point, the
adapter or domain will no longer be assigned to the
matrix_mdev->shadow_apcb or the matrix_dev->info object because they would
have been removed by vfio_ap_on_cfg_changed(). Consequently,
vfio_ap_mdev_reset_queue(q) is bypassed and kfree(q) is called
without executing vfio_ap_free_aqic_resources(). This indefinitely pins
guest memory (q->saved_iova) and leaks KVM GISC resources (q->saved_isc).

Note that resetting the queue would fail with an invalid APQN error due to
the fact the queue is not longer in the host's AP configuration; however,
it is still necessary to free the AQIC resources. The fix here is to call
vfio_ap_free_aqic_resources if the adapter or domain is neither in
matrix_mdev->shadow_apcb or matrix_dev->info.

Fixes: b9bd10c43456d ("s390/vfio-ap: do not reset queue removed from host config")
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 | 5 ++++-
 1 file changed, 4 insertions(+), 1 deletion(-)

diff --git a/drivers/s390/crypto/vfio_ap_ops.c b/drivers/s390/crypto/vfio_ap_ops.c
index 1edd0b7a3cce..bd9d239caeba 100644
--- a/drivers/s390/crypto/vfio_ap_ops.c
+++ b/drivers/s390/crypto/vfio_ap_ops.c
@@ -2533,12 +2533,15 @@ void vfio_ap_mdev_remove_queue(struct ap_device *apdev)
 	/*
 	 * If the queue is not in the host's AP configuration, then resetting
 	 * it will fail with response code 01, (APQN not valid); so, let's make
-	 * sure it is in the host's config.
+	 * sure it is in the host's config. If it is not, then free the KVM GISC
+	 * resources.
 	 */
 	if (test_bit_inv(apid, (unsigned long *)matrix_dev->info.apm) &&
 	    test_bit_inv(apqi, (unsigned long *)matrix_dev->info.aqm)) {
 		vfio_ap_mdev_reset_queue(q);
 		flush_work(&q->reset_work);
+	} else {
+		vfio_ap_free_aqic_resources(q);
 	}
 
 done:
-- 
2.53.0

Re: [PATCH v5 9/9] s390/vfio-ap: Fix memory leak when queue removed from host AP config

[sashiko-bot]

Thank you for your contribution! Sashiko AI review found 1 potential issue(s) to consider:
- [High] Incomplete fix for resource leak: `vfio_ap_mdev_reset_queue()` does not handle `AP_RESPONSE_Q_NOT_AVAIL` (0x01), causing KVM GISC/pinned memory leaks and spurious WARN_ON stack traces.
--

--- Patch [9]: [PATCH v5 9/9] s390/vfio-ap: Fix memory leak when queue removed from host AP config ---
commit 6c8828db27ae24e9b17bf29e492fb4382c470413
Author: Anthony Krowiak <akrowiak@linux.ibm.com>

s390/vfio-ap: Fix memory leak when queue removed from host AP config

This patch aims to fix a memory leak of KVM GISC resources and pinned guest
memory when an adapter or domain is removed from the host's AP configuration.
It bypasses the queue reset and directly frees the AQIC resources if the
queue is no longer in the host config.

> diff --git a/drivers/s390/crypto/vfio_ap_ops.c b/drivers/s390/crypto/vfio_ap_ops.c
> index 1edd0b7a3cce7..bd9d239caeba2 100644
> --- a/drivers/s390/crypto/vfio_ap_ops.c
> +++ b/drivers/s390/crypto/vfio_ap_ops.c
> @@ -2533,12 +2533,15 @@ void vfio_ap_mdev_remove_queue(struct ap_device *apdev)
>  	/*
>  	 * If the queue is not in the host's AP configuration, then resetting
>  	 * it will fail with response code 01, (APQN not valid); so, let's make
> -	 * sure it is in the host's config.
> +	 * sure it is in the host's config. If it is not, then free the KVM GISC
> +	 * resources.
>  	 */
>  	if (test_bit_inv(apid, (unsigned long *)matrix_dev->info.apm) &&
>  	    test_bit_inv(apqi, (unsigned long *)matrix_dev->info.aqm)) {
>  		vfio_ap_mdev_reset_queue(q);
>  		flush_work(&q->reset_work);
> +	} else {
> +		vfio_ap_free_aqic_resources(q);
[Severity: High]
Can this still trigger a WARN_ON and leak KVM GISC and pinned memory if a guest
shuts down or the mdev is removed before the physical queue is unbound?

When an adapter is removed from the host config, ap_zapq() returns 0x01
(AP_RESPONSE_Q_NOT_AVAIL). If vfio_ap_mdev_reset_queue() receives this response
(for example, during guest shutdown), it falls into the default case, logs a
WARN, and skips freeing AQIC resources:

drivers/s390/crypto/vfio_ap_ops.c:vfio_ap_mdev_reset_queue() {
	...
	default:
		WARN(true,
		     "PQAP/ZAPQ for %02x.%04x failed with invalid rc=%u\n",
		     AP_QID_CARD(q->apqn), AP_QID_QUEUE(q->apqn),
		     status.response_code);
	...
}

The driver might then clear the kvm or q->matrix_mdev pointers. Later, when
the physical queue is unbound and this else branch is reached,
vfio_ap_free_aqic_resources(q) would hit the WARN_ON checks because the
pointers are now NULL:

drivers/s390/crypto/vfio_ap_ops.c:vfio_ap_free_aqic_resources() {
	...
	if (q->saved_isc != VFIO_AP_ISC_INVALID &&
	    !WARN_ON(!(q->matrix_mdev && q->matrix_mdev->kvm))) {
	...
	if (q->saved_iova && !WARN_ON(!q->matrix_mdev)) {
	...
}

Should vfio_ap_mdev_reset_queue() be updated to explicitly handle response
code 0x01 and clean up resources there as well to ensure they are freed
before the pointers are cleared?

>  	}
>  
>  done:

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

Re: [PATCH v5 9/9] s390/vfio-ap: Fix memory leak when queue removed from host AP config

[Matthew Rosato]

On 8/12/26 4:02 PM, Anthony Krowiak wrote:
> When an adapter or domain is removed from the host's AP configuration, the
> AP bus invokes vfio_ap_on_cfg_changed() to notify the vfio_ap device
> driver. For each ap_matrix_mdev object to which the adapter or domain
> is assigned, vfio_ap_mdev_hot_unplug_cfg() is called and removes the
> adapter or domain from the matrix_mdev->shadow_apcb (i.e., the guest's AP
> configuration) and hot unplugs it if a guest is using it. The new host
> AP configuration (sans adapter or domain) is then stored in
> matrix_dev->info.
> 
> When the AP bus subsequently unbinds the physical queue devices associated
> with the adapter or domain that has been removed, it invokes
> vfio_ap_mdev_remove_queue() for each queue removed. At this point, the
> adapter or domain will no longer be assigned to the
> matrix_mdev->shadow_apcb or the matrix_dev->info object because they would
> have been removed by vfio_ap_on_cfg_changed(). Consequently,
> vfio_ap_mdev_reset_queue(q) is bypassed and kfree(q) is called
> without executing vfio_ap_free_aqic_resources(). This indefinitely pins
> guest memory (q->saved_iova) and leaks KVM GISC resources (q->saved_isc).
> 
> Note that resetting the queue would fail with an invalid APQN error due to
> the fact the queue is not longer in the host's AP configuration; however,
> it is still necessary to free the AQIC resources. The fix here is to call
> vfio_ap_free_aqic_resources if the adapter or domain is neither in
> matrix_mdev->shadow_apcb or matrix_dev->info.
> 
> Fixes: b9bd10c43456d ("s390/vfio-ap: do not reset queue removed from host config")
> 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 | 5 ++++-
>  1 file changed, 4 insertions(+), 1 deletion(-)
> 
> diff --git a/drivers/s390/crypto/vfio_ap_ops.c b/drivers/s390/crypto/vfio_ap_ops.c
> index 1edd0b7a3cce..bd9d239caeba 100644
> --- a/drivers/s390/crypto/vfio_ap_ops.c
> +++ b/drivers/s390/crypto/vfio_ap_ops.c
> @@ -2533,12 +2533,15 @@ void vfio_ap_mdev_remove_queue(struct ap_device *apdev)
>  	/*
>  	 * If the queue is not in the host's AP configuration, then resetting
>  	 * it will fail with response code 01, (APQN not valid); so, let's make
> -	 * sure it is in the host's config.
> +	 * sure it is in the host's config. If it is not, then free the KVM GISC
> +	 * resources.
>  	 */
>  	if (test_bit_inv(apid, (unsigned long *)matrix_dev->info.apm) &&
>  	    test_bit_inv(apqi, (unsigned long *)matrix_dev->info.aqm)) {
>  		vfio_ap_mdev_reset_queue(q);
>  		flush_work(&q->reset_work);
> +	} else {
> +		vfio_ap_free_aqic_resources(q);

Re: the Sashiko finding for this one:

[Severity: High]
Can this still trigger a WARN_ON and leak KVM GISC and pinned memory if
a guest shuts down or the mdev is removed before the physical queue is
unbound?

I think Sashiko has a point here...  At first I thought it was a
pre-existing issue (just another path that fails to free resources) but
I think this patch actually makes the described thread of execution a
bit worse:

Before this patch you would leak resources on the described path.

After this patch, you will leak the resources on that path, then reach
this new else statement and could potentially now WARN about the null
pointer(s) in vfio_ap_free_aqic_resources() and still not unregister the
gisc and/or unpin the saved_iova due to the null pointers.

Re: [PATCH v5 9/9] s390/vfio-ap: Fix memory leak when queue removed from host AP config

[Anthony Krowiak]

On 8/12/26 5:12 PM, Matthew Rosato wrote:
> On 8/12/26 4:02 PM, Anthony Krowiak wrote:
>> When an adapter or domain is removed from the host's AP configuration, the
>> AP bus invokes vfio_ap_on_cfg_changed() to notify the vfio_ap device
>> driver. For each ap_matrix_mdev object to which the adapter or domain
>> is assigned, vfio_ap_mdev_hot_unplug_cfg() is called and removes the
>> adapter or domain from the matrix_mdev->shadow_apcb (i.e., the guest's AP
>> configuration) and hot unplugs it if a guest is using it. The new host
>> AP configuration (sans adapter or domain) is then stored in
>> matrix_dev->info.
>>
>> When the AP bus subsequently unbinds the physical queue devices associated
>> with the adapter or domain that has been removed, it invokes
>> vfio_ap_mdev_remove_queue() for each queue removed. At this point, the
>> adapter or domain will no longer be assigned to the
>> matrix_mdev->shadow_apcb or the matrix_dev->info object because they would
>> have been removed by vfio_ap_on_cfg_changed(). Consequently,
>> vfio_ap_mdev_reset_queue(q) is bypassed and kfree(q) is called
>> without executing vfio_ap_free_aqic_resources(). This indefinitely pins
>> guest memory (q->saved_iova) and leaks KVM GISC resources (q->saved_isc).
>>
>> Note that resetting the queue would fail with an invalid APQN error due to
>> the fact the queue is not longer in the host's AP configuration; however,
>> it is still necessary to free the AQIC resources. The fix here is to call
>> vfio_ap_free_aqic_resources if the adapter or domain is neither in
>> matrix_mdev->shadow_apcb or matrix_dev->info.
>>
>> Fixes: b9bd10c43456d ("s390/vfio-ap: do not reset queue removed from host config")
>> 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 | 5 ++++-
>>   1 file changed, 4 insertions(+), 1 deletion(-)
>>
>> diff --git a/drivers/s390/crypto/vfio_ap_ops.c b/drivers/s390/crypto/vfio_ap_ops.c
>> index 1edd0b7a3cce..bd9d239caeba 100644
>> --- a/drivers/s390/crypto/vfio_ap_ops.c
>> +++ b/drivers/s390/crypto/vfio_ap_ops.c
>> @@ -2533,12 +2533,15 @@ void vfio_ap_mdev_remove_queue(struct ap_device *apdev)
>>   	/*
>>   	 * If the queue is not in the host's AP configuration, then resetting
>>   	 * it will fail with response code 01, (APQN not valid); so, let's make
>> -	 * sure it is in the host's config.
>> +	 * sure it is in the host's config. If it is not, then free the KVM GISC
>> +	 * resources.
>>   	 */
>>   	if (test_bit_inv(apid, (unsigned long *)matrix_dev->info.apm) &&
>>   	    test_bit_inv(apqi, (unsigned long *)matrix_dev->info.aqm)) {
>>   		vfio_ap_mdev_reset_queue(q);
>>   		flush_work(&q->reset_work);
>> +	} else {
>> +		vfio_ap_free_aqic_resources(q);
> Re: the Sashiko finding for this one:
>
> [Severity: High]
> Can this still trigger a WARN_ON and leak KVM GISC and pinned memory if
> a guest shuts down or the mdev is removed before the physical queue is
> unbound?
>
> I think Sashiko has a point here...  At first I thought it was a
> pre-existing issue (just another path that fails to free resources) but
> I think this patch actually makes the described thread of execution a
> bit worse:
>
> Before this patch you would leak resources on the described path.
>
> After this patch, you will leak the resources on that path, then reach
> this new else statement and could potentially now WARN about the null
> pointer(s) in vfio_ap_free_aqic_resources() and still not unregister the
> gisc and/or unpin the saved_iova due to the null pointers.

This is a legitimate concern. I will fix this via an individual patch since
this 9/9 patch will not be included in the series for the pull request.

>
>

Re: [PATCH v5 0/9] s390/vfio-ap: Fix bugs in vfio_ap device driver callback functions

[Christian Borntraeger]

Am 12.08.26 um 22:02 schrieb Anthony Krowiak:
> During review of patches by the Sashiko AI, several pre-existing bugs were
> discovered. This 9-patch series fixes those bugs.
> 
> Change log v4 => v5:
> ~~~~~~~~~~~~~~~~~~~
> Patch 8/9: Fix NULL deref in status_show() during queue probe
> * Check if q = dev_get_drvdata(&apdev->device) actually returns a pointer
>    to a vfio_ap_queue object. If not, the return AP_QUEUE_UNASSIGNED
> 
> Anthony Krowiak (9):
>    s390/vfio-ap: Fix stale do_remove flag across iterations in
>      vfio_ap_mdev_cfg_remove
>    s390/vfio-ap: Fix dereference matrix_mdev->kvm without checking for
>      NULL
>    s390/vfio-ap: Fix missing lock required to access list of
>      ap_matrix_mdev objects
>    s390/vfio-ap: Fix required lock not held during update of
>      ap_matrix_mdev object
>    s390/vfio-ap: Fix control domain removal in vfio_ap_mdev_cfg_remove
>    s390/vfio-ap: fix potential use of uninitialized apm_filtered bitmap
>    s390/vfio-ap: Fix hot-unplug skipped when last AP adapter or domain
>      removed
>    s390/vfio-ap: Fix NULL deref in status_show() during queue probe
>    s390/vfio-ap: Fix memory leak when queue removed from host AP config
> 
>   drivers/s390/crypto/vfio_ap_ops.c | 163 +++++++++++++++++++++++-------
>   1 file changed, 126 insertions(+), 37 deletions(-)
> 

So I picked 1-8. Lets do 9 later with the other open issues.