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

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

Patch 2: Fix dereference matrix_mdev->kvm without checking for NULL
* Added block comment prior to traversing the matix_dev->mdev_list to 
  explain that it is safe to traverse the list because the 
  matrix_dev->guests_lock is taken in vfio_ap_on_cfg_changed() which is
  called prior to calling vfio_ap_mdev_cfg_remove to hopefully circumvent
  repeated LLM review comments that the list can change while traversing
  it.
* Added block comment prior to adding a matrix_mdev to the
  matrix_dev->mdev_list in vfio_ap_mdev_probe() to hopefully circumvent
  repeated LLM review comments that the list can change while traversing
  it.
  
Patch 3: Fix missing lock required to access list of ap_matrix_mdev objects
* Removed setting of matrix_mdev->req_trigger and 
  matrix_mdev->cfg_chg_trigger to NULL because matrix_mdev is kzalloc's and
  to avoid setting them to NULL after exposing the device to
  userspace via the prior call to vfio_register_emulated_iommu_dev(). 
  
Patch 6: Fix potential use of uninitialized apm_filtered bitmap
* Added block comment prior to zeroizing apm_filtered bitmap to explain why
  and hopefully prevent LLM review comments about it being uninitialized.

Patch 8: Fix NULL deref in status_show() during queue probe
* Added call to release_update_locks_for_mdev() prior to calling
  sysfs_remove_group() to fix an ABBA deadlock on the error path.


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

-- 
2.53.0

[PATCH v4 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

[PATCH v4 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

[PATCH v4 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

[PATCH v4 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

[PATCH v4 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

[PATCH v4 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

[PATCH v4 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

[PATCH v4 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 | 15 +++++++++++----
 1 file changed, 11 insertions(+), 4 deletions(-)

diff --git a/drivers/s390/crypto/vfio_ap_ops.c b/drivers/s390/crypto/vfio_ap_ops.c
index 16779cfc64e8..b0454a296c67 100644
--- a/drivers/s390/crypto/vfio_ap_ops.c
+++ b/drivers/s390/crypto/vfio_ap_ops.c
@@ -2424,14 +2424,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 +2442,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 +2474,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

[PATCH v4 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 b0454a296c67..a8de98d5e71e 100644
--- a/drivers/s390/crypto/vfio_ap_ops.c
+++ b/drivers/s390/crypto/vfio_ap_ops.c
@@ -2515,12 +2515,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 v4 6/9] s390/vfio-ap: fix potential use of uninitialized apm_filtered bitmap

[sashiko-bot]

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

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

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

Re: [PATCH v4 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 uses the return value of bitmap_andnot to decide whether to update the guest APCB, bypassing the update when the last device or domain is removed.
--

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

s390/vfio-ap: Fix control domain removal in vfio_ap_mdev_cfg_remove
    
The vfio_ap_config_remove function uses the bitmap_andnot function to clear
bits from the matrix_mdev->matrix.adm bitmap. This prevents the explicitly
unplugged control domains from being removed from 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

> 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() fail to
update the guest APCB when the last device or domain is removed?

In drivers/s390/crypto/vfio_ap_ops.c:vfio_ap_mdev_hot_unplug_cfg(), the return
value of bitmap_andnot() is used to determine if the hotplug update should
occur:

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

Because bitmap_andnot() returns true only if the resulting destination bitmap
is non-empty, removing the final remaining bits will cause bitmap_andnot() to
return 0.

If a host administrator removes the last remaining assigned AP adapter or
domain from the host configuration for a specific mdev, do_hotplug remains 0.
Will this skip the vfio_ap_mdev_update_guest_apcb() call and allow the KVM
guest to retain hardware access to the unplugged resources?

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

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

Pre-existing issues:
- [High] Unchecked dereference of `matrix_mdev->kvm` in `vfio_ap_mdev_cfg_remove` and `vfio_ap_mdev_hot_plug_cfg` leading to a kernel panic for mediated devices not attached to a KVM guest.
- [High] Control domain hot unplug in `vfio_ap_mdev_cfg_remove()` is completely broken due to incorrect use of `bitmap_andnot` instead of `bitmap_and`.
- [High] Unsafe iteration over `matrix_dev->mdev_list` in `vfio_ap_mdev_cfg_remove` without holding `matrix_dev->mdevs_lock`, risking a data race.
- [High] Unsupported older AP adapters (pre-CEX4) can bypass filtering in `vfio_ap_mdev_cfg_add` if no new domains are added simultaneously.
--

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

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

This commit fixes the initialization and accumulation of the do_remove flag
in vfio_ap_mdev_cfg_remove(). Previously, the flag was never reset per
iteration, causing a positive result to carry over to subsequent matrix mdevs.
The fix sets the flag using bitmap_and() for the first check rather than
accumulating it.

> 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 is it safe to iterate over
matrix_dev->mdev_list here without holding matrix_dev->mdevs_lock?

The caller vfio_ap_on_cfg_changed() holds matrix_dev->guests_lock, but not
matrix_dev->mdevs_lock. Since mdevs_lock is acquired inside the loop and
released before the next iteration, concurrent modifications might corrupt
the list traversal if new mdevs are added in vfio_ap_mdev_probe() holding
only mdevs_lock.

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

[Severity: High]
This is a pre-existing issue, but does this code unconditionally dereference
matrix_mdev->kvm without checking for null?

If a mediated device is created but not yet attached to a kvm guest,
matrix_mdev->kvm remains initialized to null. If a host ap configuration
change occurs in this state, wouldn't this cause a kernel panic?

The same unconditional dereference appears to exist in
vfio_ap_mdev_hot_plug_cfg() when called from vfio_ap_on_cfg_changed().

>  		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 isn't a bug introduced by this patch, but looking at the next lines in
vfio_ap_mdev_cfg_remove():

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

Is it correct to use bitmap_andnot() here instead of bitmap_and()?

This computes cd_remove & ~matrix_mdev->matrix.adm, yielding domains that
were removed from the host but are explicitly not assigned to the mdev.
Later, vfio_ap_mdev_hot_unplug_cfg() checks for intersection between the
guest's active domains and cdrem, which will always fail. Should this be
bitmap_and() instead?

[Severity: High]
This isn't a bug introduced by this patch, but is it possible for unsupported
older ap adapters to bypass filtering?

In vfio_ap_mdev_cfg_add(), the code calls:

drivers/s390/crypto/vfio_ap_ops.c:vfio_ap_mdev_cfg_add() {
    ...
    vfio_ap_filter_apid_by_qtype(apm_add, aqm_add);
    ...
}

If an older adapter is added to the host configuration without any
new domains being added simultaneously (because the domains were already
present), aqm_add will be empty.

This causes the inner loop in vfio_ap_filter_apid_by_qtype() to be skipped:

drivers/s390/crypto/vfio_ap_ops.c:vfio_ap_filter_apid_by_qtype() {
    ...
    for_each_set_bit_inv(apqi, aqm, AP_DOMAINS) {
        status = ap_test_queue(AP_MKQID(apid, apqi), 1, &info);
    ...
}

Because aqm is empty, the loop evaluating adapter type does not execute,
meaning the adapter's type is never verified and it remains in apm_add.
Should the active domains be passed to the filter instead of the delta?

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

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

[sashiko-bot]

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

Pre-existing issues:
- [High] Pinned NIB pages and KVM ISC registrations are permanently leaked when an AP queue is removed from the host configuration.
--

--- Patch [7]: [PATCH v4 7/9] s390/vfio-ap: Fix hot-unplug skipped when last AP adapter or domain removed ---
commit b0a12cab04822e364a31d2095615141da73872b3
Author: Anthony Krowiak <akrowiak@linux.ibm.com>

s390/vfio-ap: Fix hot-unplug skipped when last AP adapter or domain removed

This commit fixes an issue where the guest APCB is not updated when the
last AP adapter or domain is removed. It replaces the bitmap_andnot check
with bitmap_intersects to properly detect overlaps and trigger the guest
update.

Cc: stable@vger.kernel.org

> diff --git a/drivers/s390/crypto/vfio_ap_ops.c b/drivers/s390/crypto/vfio_ap_ops.c
> --- 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,
[ ... ]
> +	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);
>  }

[Severity: High]
This is a pre-existing issue, but while reviewing the hot-unplug path I
noticed a potential resource leak in vfio_ap_mdev_remove_queue().

When an AP adapter or domain is hot-unplugged, it is removed from
matrix_dev->info during the configuration change. Later, when the AP bus
calls vfio_ap_mdev_remove_queue(), it checks the matrix_dev->info bitmaps:

drivers/s390/crypto/vfio_ap_ops.c: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);
        }

done:
        if (matrix_mdev)
                vfio_ap_unlink_queue_fr_mdev(q);

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

Because the queue is already removed from matrix_dev->info by the time this
is called, the test_bit_inv() checks evaluate to false.

Does this bypass vfio_ap_mdev_reset_queue() and permanently leak AQIC
resources like NIB pinned pages and KVM ISC registrations before the queue
is freed?

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

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

Pre-existing issues:
- [High] Hardware validation is skipped when an adapter is hotplugged without new domains, allowing unsupported hardware to be assigned.
--

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

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

This patch resolves a missing lock issue in vfio_ap_mdev_cfg_add(). It ensures
that the mdevs_lock is held while modifying the apm_add, aqm_add, and adm_add
bitmap fields. This prevents race conditions with concurrent modifications to
the matrix device.

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

[Severity: High]
This isn't a bug introduced by this patch, but does 
vfio_ap_filter_apid_by_qtype() skip hardware validation if an adapter is 
hotplugged without new domains?

In vfio_ap_filter_apid_by_qtype(), if aqm is empty:

drivers/s390/crypto/vfio_ap_ops.c:vfio_ap_filter_apid_by_qtype() {
...
    for_each_set_bit_inv(apid, apm, AP_DEVICES) {
        apid_cleared = false;

        for_each_set_bit_inv(apqi, aqm, AP_DOMAINS) {
            status = ap_test_queue(AP_MKQID(apid, apqi), 1, &info);
...
}

The inner loop iterating over aqm (which maps to aqm_add) will not execute
if no domains were added during the config change. As a result, the hardware
is never queried with ap_test_queue(), and unsupported older adapters (older
than CEX4) are not filtered out from apm_add.

Can this allow unsupported hardware to be assigned to a KVM guest if a host
administrator hotplugs an older AP adapter without simultaneously hotplugging
new domains?

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

[ ... ]

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

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

Pre-existing issues:
- [High] The patch attempts to fix a TOCTOU race by holding update locks during sysfs group creation, but leaves the exact same race window open on the allocation error path, leading to a NULL pointer dereference.
- [High] Missing flush or cancellation of `q->reset_work` in `vfio_ap_mdev_remove_queue` leads to a Use-After-Free.
--

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

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

This commit acquires the update locks before calling sysfs_create_group()
during queue probe to prevent a NULL pointer dereference in a concurrent
read of the status attribute. It also optimizes APQN retrieval.

> diff --git a/drivers/s390/crypto/vfio_ap_ops.c b/drivers/s390/crypto/vfio_ap_ops.c
> --- a/drivers/s390/crypto/vfio_ap_ops.c
> +++ b/drivers/s390/crypto/vfio_ap_ops.c

[ ... ]

> @@ -2472,8 +2474,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;

[Severity: High]
This is a pre-existing issue, but does releasing the update locks before
removing the sysfs group reopen the race window on the allocation error path?

If kzalloc_obj() fails, the error path releases guests_lock before calling
sysfs_remove_group(). This immediately unblocks any concurrent readers of
status_show() while the sysfs file still exists.

Once unblocked, status_show() will retrieve NULL from dev_get_drvdata() and
pass it to vfio_ap_mdev_for_queue(), which unconditionally dereferences it:

vfio_ap_ops.c:status_show()
    q = dev_get_drvdata(&apdev->device);
    matrix_mdev = vfio_ap_mdev_for_queue(q);

vfio_ap_ops.c:vfio_ap_mdev_for_queue()
    unsigned long apid = AP_QID_CARD(q->apqn);

Should sysfs_remove_group() be called before release_update_locks_for_mdev()
here to prevent this NULL pointer dereference?

> +
> +err_release_locks:
> +	release_update_locks_for_mdev(matrix_mdev);
> +	return ret;
>  }
>  
>  void vfio_ap_mdev_remove_queue(struct ap_device *apdev)
>  {

[ ... ]

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

[Severity: High]
This is a pre-existing issue, but does this conditional bypass of
flush_work() create a use-after-free risk?

If a hardware hot-unplug races with a background reset operation, and the
queue is not in the host's AP configuration, the teardown path skips flushing
the reset_work and frees the queue object.

Meanwhile, the background worker continues executing and accesses the freed
structure:

vfio_ap_ops.c:apq_reset_check()
    q = container_of(reset_work, struct vfio_ap_queue, reset_work);
    memcpy(&status, &q->reset_status, sizeof(status));
    ...
    status = ap_tapq(q->apqn, NULL);

Should flush_work() or cancel_work_sync() be called unconditionally before
kfree(q) to prevent this memory corruption?

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

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

Pre-existing issues:
- [High] Concurrent modification and traversal of the `matrix_dev->mdev_list` without a shared lock leads to list corruption.
- [High] Uninitialized stack variable `apm_filtered` causes arbitrary queue resets.
- [High] Use-after-free on `kvm->lock` inside `vfio_ap_mdev_unset_kvm()`.
--

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

This commit fixes a potential NULL pointer dereference in the callback
functions vfio_ap_on_cfg_changed and vfio_ap_on_scan_complete. It adds
checks to ensure matrix_mdev->kvm is not NULL before acquiring the
matrix_mdev->kvm->lock mutex. The lock acquisition in
vfio_ap_mdev_hot_plug_cfg is also moved to the calling functions to
satisfy lock ordering requirements.

> 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
> @@ -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) {

[Severity: High]
This isn't a bug introduced by this patch, but does the traversal of
matrix_dev->mdev_list here race with list modifications?

In vfio_ap_mdev_probe(), a newly created mediated device is added to the list
while holding only mdevs_lock:

	mutex_lock(&matrix_dev->mdevs_lock);
	list_add(&matrix_mdev->node, &matrix_dev->mdev_list);
	mutex_unlock(&matrix_dev->mdevs_lock);

Here in vfio_ap_mdev_cfg_remove() (and similarly in 
vfio_ap_on_scan_complete()), the list is traversed while holding only
guests_lock. Since the writer and readers do not acquire a shared lock, can
this concurrent execution lead to a corrupted doubly linked list?

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

[ ... ]

> @@ -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;
>  

[Severity: High]
This is a pre-existing issue, but could apm_filtered remain uninitialized
before being consumed?

If a host AP configuration change adds only control domains, filter_adapters
and filter_domains will evaluate to false:

	filter_adapters = bitmap_intersects(matrix_mdev->matrix.apm,
					    matrix_mdev->apm_add, AP_DEVICES);
	filter_domains = bitmap_intersects(matrix_mdev->matrix.aqm,
					   matrix_mdev->aqm_add, AP_DOMAINS);

This causes the code to skip calling vfio_ap_mdev_filter_matrix(), which
normally initializes apm_filtered.

The uninitialized stack variable is then passed to reset_queues_for_apids():

	reset_queues_for_apids(matrix_mdev, apm_filtered);

Does this cause reset_queues_for_apids() to iterate over random stack garbage
and reset unintended queues?

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

[ ... ]

Additionally, there appears to be another pre-existing issue in
vfio_ap_mdev_unset_kvm() which isn't part of this diff:

[Severity: High]
Is there a potential use-after-free on kvm->lock in vfio_ap_mdev_unset_kvm()?

In vfio_ap_mdev_unset_kvm(), the code acquires kvm->lock via 
get_update_locks_for_kvm(kvm) and later drops the KVM reference:

	get_update_locks_for_kvm(kvm);
	kvm_arch_crypto_clear_masks(kvm);
	vfio_ap_mdev_reset_queues(matrix_mdev);
	kvm_put_kvm(kvm);
	matrix_mdev->kvm = NULL;
	release_update_locks_for_kvm(kvm);

If userspace has already closed the KVM VM file descriptor, kvm_put_kvm()
could drop the final reference and free the kvm structure.

Immediately after this, release_update_locks_for_kvm(kvm) attempts to unlock
the mutex residing in the freed memory:

	if (kvm)
		mutex_unlock(&kvm->lock);

Could this result in a use-after-free on kvm->lock?

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

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

Pre-existing issues:
- [Critical] The sysfs file 'status' is exposed to userspace before the device's driver data is fully initialized, leading to a potential NULL pointer dereference in status_show().
- [High] A race condition in vfio_ap_mdev_probe() allows userspace to bypass the 'one mdev per KVM' isolation restriction.
- [High] Eventfd contexts for `req_trigger` and `cfg_chg_trigger` are leaked when the mediated device is closed or removed.
--

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

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

This patch fixes list traversal synchronization for ap_matrix_mdev objects
by adding guests_lock around mdev_list access. It also removes redundant
zero-initialization of eventfd triggers in the probe function.

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

[Severity: High]
This isn't a bug introduced by this patch, but does exposing the VFIO
device to userspace here create a race condition?

vfio_register_emulated_iommu_dev() makes the device visible to userspace
before it is added to matrix_dev->mdev_list under guests_lock. Could
userspace open multiple mdevs concurrently, allowing them to bypass the 'one
mdev per KVM' isolation restriction since vfio_ap_mdev_set_kvm() won't see
the other devices in the list yet?

>  	if (ret)
>  		goto err_put_vdev;
> -	matrix_mdev->req_trigger = NULL;
> -	matrix_mdev->cfg_chg_trigger = NULL;

[Severity: High]
This is a pre-existing issue, but since you are touching the eventfd triggers
here, are they properly cleaned up when the device is closed?

Userspace can set IRQ triggers via VFIO_DEVICE_SET_IRQS, which calls
eventfd_ctx_fdget() and takes a reference. If the user closes the device
without unregistering the IRQs, shouldn't vfio_ap_mdev_close_device() or
vfio_ap_mdev_remove() call eventfd_ctx_put() for req_trigger and
cfg_chg_trigger to prevent a memory and file descriptor leak?

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

[ ... ]

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

[Severity: Critical]
This is a pre-existing issue, but is it possible for dev_get_drvdata() to
return a NULL pointer here if the status sysfs file is read concurrently
with device probing?

During vfio_ap_mdev_probe_queue(), sysfs_create_group() creates the status
attribute before dev_set_drvdata() initializes the driver data. Can a
concurrent read of the status file fetch a NULL pointer in status_show() and
pass it to vfio_ap_mdev_for_queue(), causing a crash when it dereferences
q->apqn?

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

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

Re: [PATCH v4 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] The patch introduces potential `WARN_ON` kernel splats and fails to fully fix the resource leak if the mdev is destroyed or the queue is unassigned before the AP bus invokes `vfio_ap_mdev_remove_queue()`.
--

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

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

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.

> diff --git a/drivers/s390/crypto/vfio_ap_ops.c b/drivers/s390/crypto/vfio_ap_ops.c
> index b0454a296c67e..a8de98d5e71ed 100644
> --- a/drivers/s390/crypto/vfio_ap_ops.c
> +++ b/drivers/s390/crypto/vfio_ap_ops.c
> @@ -2515,12 +2515,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 bypass the resource cleanup and cause kernel splats if the mdev is 
destroyed or the queue is unassigned before this point?

If userspace unassigns the queue or QEMU closes the mdev, paths like
vfio_ap_mdev_unset_kvm() or unassign_adapter_store() will attempt to reset 
the queue by calling vfio_ap_mdev_reset_queue().

Since the adapter is no longer in the host config, ap_zapq() returns 0x01
(AP_RESPONSE_Q_NOT_AVAIL). However, vfio_ap_mdev_reset_queue() does not handle
this response code in its switch statement. This bypasses the synchronous
cleanup in vfio_ap_free_aqic_resources(), leaving q->saved_isc and
q->saved_iova valid.

The cleanup routines then proceed to clear q->matrix_mdev or
q->matrix_mdev->kvm.

When the AP bus later asynchronously calls vfio_ap_mdev_remove_queue(), this
newly added else block is reached.

Inside vfio_ap_free_aqic_resources():

	if (q->saved_isc != VFIO_AP_ISC_INVALID &&
	    !WARN_ON(!(q->matrix_mdev && q->matrix_mdev->kvm))) {
		kvm_s390_gisc_unregister(q->matrix_mdev->kvm, q->saved_isc);
		...
	if (q->saved_iova && !WARN_ON(!q->matrix_mdev)) {
		vfio_unpin_pages(&q->matrix_mdev->vdev, q->saved_iova, 1);

Because q->matrix_mdev or q->matrix_mdev->kvm is now NULL, the WARN_ON checks
evaluate to true. This triggers a kernel splat and bypasses the actual cleanup
calls, leaking the KVM GISC resources and pinned pages.

>  	}
>  
>  done:

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

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

[Matthew Rosato]

On 8/12/26 1:09 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>
> ---
>  drivers/s390/crypto/vfio_ap_ops.c | 15 +++++++++++----
>  1 file changed, 11 insertions(+), 4 deletions(-)
> 
> diff --git a/drivers/s390/crypto/vfio_ap_ops.c b/drivers/s390/crypto/vfio_ap_ops.c
> index 16779cfc64e8..b0454a296c67 100644
> --- a/drivers/s390/crypto/vfio_ap_ops.c
> +++ b/drivers/s390/crypto/vfio_ap_ops.c
> @@ -2424,14 +2424,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 +2442,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 +2474,13 @@ int vfio_ap_mdev_probe_queue(struct ap_device *apdev)
>  	return ret;
>  
>  err_remove_group:
> +	release_update_locks_for_mdev(matrix_mdev);

I think now you've just shrunk the window instead of totally closed it?

Can't sysfs still be accessed between this release and the remove_group?

Do you think adding an additional NULL check against dev_get_drvdata()
in status_show() and treating the NULL case like the other error
conditions would be enough to cover that window? 

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

[Anthony Krowiak]

On 8/12/26 1:58 PM, Matthew Rosato wrote:
> On 8/12/26 1:09 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>
>> ---
>>   drivers/s390/crypto/vfio_ap_ops.c | 15 +++++++++++----
>>   1 file changed, 11 insertions(+), 4 deletions(-)
>>
>> diff --git a/drivers/s390/crypto/vfio_ap_ops.c b/drivers/s390/crypto/vfio_ap_ops.c
>> index 16779cfc64e8..b0454a296c67 100644
>> --- a/drivers/s390/crypto/vfio_ap_ops.c
>> +++ b/drivers/s390/crypto/vfio_ap_ops.c
>> @@ -2424,14 +2424,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 +2442,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 +2474,13 @@ int vfio_ap_mdev_probe_queue(struct ap_device *apdev)
>>   	return ret;
>>   
>>   err_remove_group:
>> +	release_update_locks_for_mdev(matrix_mdev);
> I think now you've just shrunk the window instead of totally closed it?

Since that locks must be released before the call to sysfs_remove_group()
to avoid an ABBA deadlock, it is not possible to totally close it.

>
> Can't sysfs still be accessed between this release and the remove_group?

That possibility is incredibly remote, but I suppose anything is possible.

>
> Do you think adding an additional NULL check against dev_get_drvdata()
> in status_show() and treating the NULL case like the other error
> conditions would be enough to cover that window?

It would cover that window, but that function does not return any error, so
we'd have to decide what makes the most sense between the following
three values, or create new one to indicate we don't know the status.

AP_QUEUE_ASSIGNED
AP_QUEUE_UNASSIGNED
AP_QUEUE_IN_USE

>   
>
>
>

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

[Anthony Krowiak]

On 8/12/26 1:28 PM, sashiko-bot@kernel.org wrote:
> Thank you for your contribution! Sashiko AI review found 3 potential issue(s) to consider:
>
> Pre-existing issues:
> - [Critical] The sysfs file 'status' is exposed to userspace before the device's driver data is fully initialized, leading to a potential NULL pointer dereference in status_show().
> - [High] A race condition in vfio_ap_mdev_probe() allows userspace to bypass the 'one mdev per KVM' isolation restriction.
> - [High] Eventfd contexts for `req_trigger` and `cfg_chg_trigger` are leaked when the mediated device is closed or removed.
> --
>
> --- Patch [3]: [PATCH v4 3/9] s390/vfio-ap: Fix missing lock required to access list of ap_matrix_mdev objects ---
> commit b763c5c7bf7a38cb1fc2e2da2ff0df92d3ae360c
> Author: Anthony Krowiak <akrowiak@linux.ibm.com>
>
> s390/vfio-ap: Fix missing lock required to access list of ap_matrix_mdev objects
>
> This patch fixes list traversal synchronization for ap_matrix_mdev objects
> by adding guests_lock around mdev_list access. It also removes redundant
> zero-initialization of eventfd triggers in the probe function.
>
>> 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);
> [Severity: High]
> This isn't a bug introduced by this patch, but does exposing the VFIO
> device to userspace here create a race condition?
>
> vfio_register_emulated_iommu_dev() makes the device visible to userspace
> before it is added to matrix_dev->mdev_list under guests_lock. Could
> userspace open multiple mdevs concurrently, allowing them to bypass the 'one
> mdev per KVM' isolation restriction since vfio_ap_mdev_set_kvm() won't see
> the other devices in the list yet?
>
>>   	if (ret)
>>   		goto err_put_vdev;
>> -	matrix_mdev->req_trigger = NULL;
>> -	matrix_mdev->cfg_chg_trigger = NULL;
> [Severity: High]
> This is a pre-existing issue, but since you are touching the eventfd triggers
> here, are they properly cleaned up when the device is closed?
>
> Userspace can set IRQ triggers via VFIO_DEVICE_SET_IRQS, which calls
> eventfd_ctx_fdget() and takes a reference. If the user closes the device
> without unregistering the IRQs, shouldn't vfio_ap_mdev_close_device() or
> vfio_ap_mdev_remove() call eventfd_ctx_put() for req_trigger and
> cfg_chg_trigger to prevent a memory and file descriptor leak?

The other concerns are fixed in this series, but this concern is not. This
needs to be fixed.

>
>> +
>> +	/*
>> +	 * 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;
> [ ... ]
>
>> @@ -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);
> [Severity: Critical]
> This is a pre-existing issue, but is it possible for dev_get_drvdata() to
> return a NULL pointer here if the status sysfs file is read concurrently
> with device probing?
>
> During vfio_ap_mdev_probe_queue(), sysfs_create_group() creates the status
> attribute before dev_set_drvdata() initializes the driver data. Can a
> concurrent read of the status file fetch a NULL pointer in status_show() and
> pass it to vfio_ap_mdev_for_queue(), causing a crash when it dereferences
> q->apqn?
>
>>   	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;
>>   }

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

[Matthew Rosato]

>>
>> Do you think adding an additional NULL check against dev_get_drvdata()
>> in status_show() and treating the NULL case like the other error
>> conditions would be enough to cover that window?
> 
> It would cover that window, but that function does not return any error, so
> we'd have to decide what makes the most sense between the following
> three values, or create new one to indicate we don't know the status.
> 
> AP_QUEUE_ASSIGNED
> AP_QUEUE_UNASSIGNED
> AP_QUEUE_IN_USE
> 

If there's no drvdata then it can't be assigned, right?  So maybe
AP_QUEUE_UNASSIGNED?  Unless you can think of a reason it would not work?