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

[PATCH v3 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 v2 => v3:
~~~~~~~~~~~~~~~~~~~
Patch 3: Fix use of wrong lock in mdev probe function
Patch 7: Fix required lock not held during display of sysfs status
attribute 
* Squashed these two patches into a new patch entitled "Fix missing lock
  required to access list of ap_matrix_mdev objects"
* Added two new patches that fix pre-existing bugs:
  ~ 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

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

-- 
2.53.0

[PATCH v3 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 v3 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 | 33 ++++++++++++++++++++++---------
 1 file changed, 24 insertions(+), 9 deletions(-)

diff --git a/drivers/s390/crypto/vfio_ap_ops.c b/drivers/s390/crypto/vfio_ap_ops.c
index 845c86ba8bc3..e382e5a1cb99 100644
--- a/drivers/s390/crypto/vfio_ap_ops.c
+++ b/drivers/s390/crypto/vfio_ap_ops.c
@@ -2606,7 +2606,13 @@ static void vfio_ap_mdev_cfg_remove(unsigned long *ap_remove,
 	int do_remove;
 
 	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 +2630,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 +2828,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 +2845,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 +2855,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 v3 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.

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>
---
 drivers/s390/crypto/vfio_ap_ops.c | 8 ++++++--
 1 file changed, 6 insertions(+), 2 deletions(-)

diff --git a/drivers/s390/crypto/vfio_ap_ops.c b/drivers/s390/crypto/vfio_ap_ops.c
index e382e5a1cb99..5c14ded4fd8e 100644
--- a/drivers/s390/crypto/vfio_ap_ops.c
+++ b/drivers/s390/crypto/vfio_ap_ops.c
@@ -803,9 +803,9 @@ static int vfio_ap_mdev_probe(struct mdev_device *mdev)
 	matrix_mdev->req_trigger = NULL;
 	matrix_mdev->cfg_chg_trigger = NULL;
 	dev_set_drvdata(&mdev->dev, matrix_mdev);
-	mutex_lock(&matrix_dev->mdevs_lock);
+	mutex_lock(&matrix_dev->guests_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 +2297,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 +2318,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 +2346,7 @@ static ssize_t status_show(struct device *dev,
 	}
 
 	mutex_unlock(&matrix_dev->mdevs_lock);
+	mutex_unlock(&matrix_dev->guests_lock);
 
 	return nchars;
 }
-- 
2.53.0

[PATCH v3 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 5c14ded4fd8e..8a1ecb891504 100644
--- a/drivers/s390/crypto/vfio_ap_ops.c
+++ b/drivers/s390/crypto/vfio_ap_ops.c
@@ -2760,12 +2760,20 @@ 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);
 
 	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 v3 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 8a1ecb891504..b6d0c988cc81 100644
--- a/drivers/s390/crypto/vfio_ap_ops.c
+++ b/drivers/s390/crypto/vfio_ap_ops.c
@@ -2625,9 +2625,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 v3 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 | 2 ++
 1 file changed, 2 insertions(+)

diff --git a/drivers/s390/crypto/vfio_ap_ops.c b/drivers/s390/crypto/vfio_ap_ops.c
index b6d0c988cc81..cdc9bf5c5e53 100644
--- a/drivers/s390/crypto/vfio_ap_ops.c
+++ b/drivers/s390/crypto/vfio_ap_ops.c
@@ -2840,6 +2840,8 @@ 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;
 
+	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 v3 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 cdc9bf5c5e53..cb1e5db69d8a 100644
--- a/drivers/s390/crypto/vfio_ap_ops.c
+++ b/drivers/s390/crypto/vfio_ap_ops.c
@@ -2563,24 +2563,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 v3 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 | 12 ++++++++----
 1 file changed, 8 insertions(+), 4 deletions(-)

diff --git a/drivers/s390/crypto/vfio_ap_ops.c b/drivers/s390/crypto/vfio_ap_ops.c
index cb1e5db69d8a..3652ba792be7 100644
--- a/drivers/s390/crypto/vfio_ap_ops.c
+++ b/drivers/s390/crypto/vfio_ap_ops.c
@@ -2419,14 +2419,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) {
@@ -2434,11 +2437,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);
@@ -2468,6 +2470,8 @@ int vfio_ap_mdev_probe_queue(struct ap_device *apdev)
 
 err_remove_group:
 	sysfs_remove_group(&apdev->device.kobj, &vfio_queue_attr_group);
+err_release_locks:
+	release_update_locks_for_mdev(matrix_mdev);
 	return ret;
 }
 
-- 
2.53.0

[PATCH v3 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>
---
 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 3652ba792be7..faab5e9179ff 100644
--- a/drivers/s390/crypto/vfio_ap_ops.c
+++ b/drivers/s390/crypto/vfio_ap_ops.c
@@ -2507,12 +2507,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 v3 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/20260812115139.576303-1-akrowiak@linux.ibm.com?part=7

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

[sashiko-bot]

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

Pre-existing issues:
- [High] Potential subsystem-wide hang due to an unbounded infinite loop in `apq_reset_check()` while holding global mutexes.
--

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

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

This commit fixes a bug where the apm_filtered bitmap was used uninitialized
if a hot plug configuration change only added control domains. It zeroes the
bitmap using bitmap_zero() at the start of the function to prevent using
stack garbage.

Cc: stable@vger.kernel.org

> diff --git a/drivers/s390/crypto/vfio_ap_ops.c b/drivers/s390/crypto/vfio_ap_ops.c
> index b6d0c988cc810..cdc9bf5c5e53d 100644
> --- a/drivers/s390/crypto/vfio_ap_ops.c
> +++ b/drivers/s390/crypto/vfio_ap_ops.c
> @@ -2840,6 +2840,8 @@ 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;
>  
> +	bitmap_zero(apm_filtered, AP_DEVICES);
> +

[Severity: High]
This isn't a bug introduced by this patch, but can the subsystem hang if the
hardware continuously returns -EBUSY?

Looking at apq_reset_check(), there is a loop that continues indefinitely
without a timeout bounds:

drivers/s390/crypto/vfio_ap_ops.c:apq_reset_check() {
	while (true) {
		msleep(AP_RESET_INTERVAL);
		elapsed += AP_RESET_INTERVAL;
		status = ap_tapq(q->apqn, NULL);
		ret = apq_status_check(q->apqn, &status);
		if (ret == -EIO)
			return;
		if (ret == -EBUSY) {
			pr_notice_ratelimited(WAIT_MSG, elapsed,...);
}

If a user process triggers a reset, it queues a reset and subsequently waits
for it using flush_work() while holding the global matrix_dev->mdevs_lock
and matrix_dev->guests_lock:

drivers/s390/crypto/vfio_ap_ops.c:vfio_ap_mdev_reset_queues() {
	hash_for_each(matrix_mdev->qtable.queues, loop_cursor, q, mdev_qnode) {
		flush_work(&q->reset_work);
}

Could this create a permanent kernel thread hang that persistently holds
global locks if the hardware is unresponsive or repeatedly returns
AP_RESPONSE_BUSY?

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

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

Re: [PATCH v3 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] Unplugging the last AP adapter, domain, or control domain fails to update the KVM guest's APCB.
--

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

s390/vfio-ap: Fix control domain removal in vfio_ap_mdev_cfg_remove

This commit resolves an issue in vfio_ap_mdev_cfg_remove where the use of
bitmap_andnot prevented explicitly unplugged control domains from being
removed from the KVM guest. The fix correctly changes the bitwise operation
to use bitmap_and instead.

Cc: stable@vger.kernel.org

> diff --git a/drivers/s390/crypto/vfio_ap_ops.c b/drivers/s390/crypto/vfio_ap_ops.c
> index 8a1ecb8915044..b6d0c988cc810 100644
> --- a/drivers/s390/crypto/vfio_ap_ops.c
> +++ b/drivers/s390/crypto/vfio_ap_ops.c
> @@ -2625,9 +2625,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 this function call successfully
update the KVM guest APCB when the very last hardware resource is
unplugged?

Looking inside vfio_ap_mdev_hot_unplug_cfg(), it uses the return value of
bitmap_andnot() to determine if an update is required:

drivers/s390/crypto/vfio_ap_ops.c:vfio_ap_mdev_hot_unplug_cfg() {
    ...
	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 false when the resulting bitmap is empty,
clearing the final bit from the shadow APCB returns 0.

If the last AP adapter, domain, or control domain is unplugged, do_hotplug
evaluates to 0 and the vfio_ap_mdev_update_guest_apcb() call is completely
bypassed.

Could this allow a guest to retain architectural access to hardware that has
been explicitly removed and potentially reassigned to another driver?

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

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

Pre-existing issues:
- [High] Pre-existing TOCTOU race condition in `vfio_ap_mdev_probe_queue` allowing NULL pointer dereference in `status_show`.
--

--- Patch [3]: [PATCH v3 3/9] s390/vfio-ap: Fix missing lock required to access list of ap_matrix_mdev objects ---
commit 431e0fcd6b2e0bbf72d273210e68aa6e049eacc3
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 missing locks needed when traversing or modifying the
ap_matrix_mdev object list in the vfio_ap driver. It ensures guests_lock
is properly held during probe and status reads to prevent concurrent list
access issues.

> diff --git a/drivers/s390/crypto/vfio_ap_ops.c b/drivers/s390/crypto/vfio_ap_ops.c
> index e382e5a1cb996..5c14ded4fd8ee 100644
> --- a/drivers/s390/crypto/vfio_ap_ops.c
> +++ b/drivers/s390/crypto/vfio_ap_ops.c

[ ... ]

> @@ -2316,6 +2318,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);

[Severity: High]
This is a pre-existing issue, but can this lead to a NULL pointer
dereference if the sysfs file is read concurrently during probe?

Looking at vfio_ap_mdev_probe_queue(), the sysfs group is created before
the driver data is initialized and set:

drivers/s390/crypto/vfio_ap_ops.c:vfio_ap_mdev_probe_queue() {
    ...
    ret = sysfs_create_group(&apdev->device.kobj, &vfio_queue_attr_group);
    if (ret)
        return ret;

    q = kzalloc(sizeof(*q), GFP_KERNEL);
    ...
    dev_set_drvdata(&apdev->device, q);
}

If status_show() is called right after the sysfs group is created but
before dev_set_drvdata() is called, dev_get_drvdata() will return NULL.

The NULL pointer is then passed directly to vfio_ap_mdev_for_queue(), which
unconditionally dereferences it:

drivers/s390/crypto/vfio_ap_ops.c:vfio_ap_mdev_for_queue() {
    ...
    unsigned long apid = AP_QID_CARD(q->apqn);
    ...
}

Should there be a check for q being NULL in status_show(), or should the
sysfs group creation be moved after the queue is fully initialized?

>  
>  	/* If the queue is assigned to the matrix mediated device, then
>  	 * determine whether it is passed through to a guest; otherwise,

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

Re: [PATCH v3 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] Uninitialized stack-allocated bitmap (`apm_filtered`) is passed to `reset_queues_for_apids`.
- [High] Missing matrix_dev->mdevs_lock acquisition in vfio_ap_mdev_cfg_add() causing data race on matrix_mdev fields.
- [High] Unsafe iteration over matrix_dev->mdev_list without holding mdevs_lock.
--

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

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

This commit adds NULL checks for matrix_mdev->kvm before attempting to
acquire the matrix_mdev->kvm->lock mutex in the configuration change and
scan complete callbacks. This prevents a potential NULL pointer dereference
if the kvm pointer has not been initialized.

> --- a/drivers/s390/crypto/vfio_ap_ops.c
> +++ b/drivers/s390/crypto/vfio_ap_ops.c
> @@ -2606,7 +2606,13 @@ static void vfio_ap_mdev_cfg_remove(unsigned long *ap_remove,
>  	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
mdev_list holding only matrix_dev->guests_lock here?

Looking at vfio_ap_mdev_probe(), it appears new entries are added to
this list holding only matrix_dev->mdevs_lock (without taking guests_lock).

Could concurrent probing during an AP bus scan modify the list while we are
traversing it locklessly here, leading to list corruption?

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

[Severity: High]
This is a pre-existing issue, but does vfio_ap_mdev_cfg_add() also need
matrix_dev->mdevs_lock protection like this?

vfio_ap_mdev_cfg_add() is called right after vfio_ap_mdev_cfg_remove() from
vfio_ap_on_cfg_changed(), which only holds matrix_dev->guests_lock. It
modifies matrix_mdev->apm_add, aqm_add, and adm_add, and reads
matrix_mdev->matrix.

Could concurrent modifications race without mdevs_lock being held and
corrupt the guest AP configuration bitmaps in vfio_ap_mdev_cfg_add()?

[ ... ]
> @@ -2821,9 +2828,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 does apm_filtered need to be zeroed or
initialized before use?

If vfio_ap_on_scan_complete() is called with non-empty adm_add but empty
apm_add and aqm_add, filter_adapters and filter_domains are set to false.
This bypasses vfio_ap_mdev_filter_matrix(), which normally initializes
apm_filtered.

Then the uninitialized stack variable is passed down:

    reset_queues_for_apids(matrix_mdev, apm_filtered);

Could this cause reset_queues_for_apids() to iterate over random stack
garbage bits and inadvertently reset arbitrary AP queues?

[ ... ]
>  void vfio_ap_on_scan_complete(struct ap_config_info *new_config_info,
> @@ -2854,15 +2855,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 isn't a bug introduced by this patch, but similar to the loop in
vfio_ap_mdev_cfg_remove() above, does this iteration also need
matrix_dev->mdevs_lock to prevent list corruption against concurrent calls
to vfio_ap_mdev_probe()?

[ ... ]

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

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

Pre-existing issues:
- [High] Data race due to iterating `mdev_list` without holding `mdevs_lock` across the entire loop.
- [High] Unconditional dereference of `matrix_mdev->kvm` when acquiring the KVM lock.
- [High] Incorrect use of `bitmap_andnot` instead of `bitmap_and` for calculating `cdrem` prevents control domains from being hot-unplugged.
- [High] Misuse of `bitmap_andnot` return value causes missed and spurious hotplug events in `vfio_ap_mdev_hot_unplug_cfg`.
- [Medium] Incorrect bitmap size constants (`AP_DEVICES` vs `AP_DOMAINS`) passed to bitmap operations.
--

--- Patch [1]: [PATCH v3 1/9] s390/vfio-ap: Fix stale do_remove flag across iterations in vfio_ap_mdev_cfg_remove ---
commit 9ff3dc11652e82272549805f96671e76c090b814
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 initialized to zero but
never reset at the start of each iteration. Because do_remove is accumulated
with bitwise OR across iterations, a positive result from one mdev carries
over to subsequent mdevs. This commit fixes the issue by setting the
do_remove flag directly with the first call to 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 is a pre-existing issue, but does iterating over mdev_list here while
repeatedly dropping and reacquiring mdevs_lock inside the loop create a data
race?

Because mdevs_lock is not held when the iterator advances, couldn't a
concurrent call to vfio_ap_mdev_probe() modify mdev_list, leading to
list corruption or use-after-free?

vfio_ap_mdev_probe() adds to this list holding only 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);

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

[Severity: High]
This isn't a bug introduced by this patch, but can matrix_mdev->kvm be NULL
here?

When an mdev is created via sysfs, matrix_mdev->kvm appears to be initialized
to NULL and is only assigned when the mdev is opened by KVM. If a host AP
configuration change occurs before the mdev is attached to a guest, couldn't
this unconditional dereference cause a host kernel panic?

(Note: similar unconditional dereferences appear in
vfio_ap_mdev_hot_plug_cfg)

>  		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 does the calculation for cdrem just below
this diff use the wrong bitwise operation?

    do_remove |= bitmap_andnot(cdrem, cd_remove,
                               matrix_mdev->matrix.adm,
                               AP_DOMAINS);

By using bitmap_andnot (which computes cd_remove & ~matrix_mdev->matrix.adm),
isn't this incorrectly identifying domains removed from the host that are not
assigned to this guest? Shouldn't this be an intersection (bitmap_and) to
identify assigned domains being removed, just like the calculations for aprem
and aqrem?


[Severity: High]
This isn't a bug introduced by this patch, but is the return value of
bitmap_andnot() being misused in vfio_ap_mdev_hot_unplug_cfg()?

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

The standard bitmap_andnot() function returns true if the resulting
destination bitmap is non-empty, not whether bits were actually changed.

If the last device is removed from the guest's shadow APCB, the resulting
bitmap is empty, so bitmap_andnot() returns false and the guest is never
notified. Conversely, if a device is removed that wasn't in the shadow APCB
but the guest has other devices, it returns true, triggering a spurious
update.


[Severity: Medium]
This is a pre-existing issue, but are the wrong size constants being passed to
bitmap operations for domains?

In vfio_ap_mdev_hot_unplug_cfg():

    do_hotplug |= bitmap_andnot(matrix_mdev->shadow_apcb.aqm,
                                matrix_mdev->shadow_apcb.aqm,
                                aqrem, AP_DEVICES);

And in vfio_ap_mdev_cfg_add():

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

Since aqm and adm represent domains, shouldn't these use AP_DOMAINS instead
of AP_DEVICES? This could lead to out-of-bounds memory accesses if the
definitions of AP_DEVICES and AP_DOMAINS ever diverge.

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

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

[sashiko-bot]

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

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

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

Re: [PATCH v3 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:
- [High] ABBA deadlock in the probe error path between kernfs active reference and guests_lock.
--

--- Patch [8]: [PATCH v3 8/9] s390/vfio-ap: Fix NULL deref in status_show() during queue probe ---
commit dd5ddcd73e238630c53e21354f9251a158e8bee2
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() during queue
probe by acquiring update locks before creating the sysfs attribute group.
This ensures any concurrent read blocks until driver data is set and locks
are released.

> diff --git a/drivers/s390/crypto/vfio_ap_ops.c b/drivers/s390/crypto/vfio_ap_ops.c
> index cb1e5db69d8a7..3652ba792be73 100644
> --- a/drivers/s390/crypto/vfio_ap_ops.c
> +++ b/drivers/s390/crypto/vfio_ap_ops.c
> @@ -2419,14 +2419,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) {
> @@ -2434,11 +2437,10 @@ int vfio_ap_mdev_probe_queue(struct ap_device *apdev)
>  		goto err_remove_group;
>  	}
[ ... ]
> @@ -2468,6 +2470,8 @@ int vfio_ap_mdev_probe_queue(struct ap_device *apdev)
>  
>  err_remove_group:
>  	sysfs_remove_group(&apdev->device.kobj, &vfio_queue_attr_group);

[Severity: High]
Does this create an ABBA deadlock on the error path?

If kzalloc_obj() fails, we jump to err_remove_group and call
sysfs_remove_group() while still holding matrix_dev->guests_lock (acquired
via get_update_locks_by_apqn()).

If a concurrent reader opens the 'status' sysfs file before we hit the error
path, it acquires a kernfs active reference and blocks on:

status_show()
    mutex_lock(&matrix_dev->guests_lock);

Then sysfs_remove_group() blocks in kernfs_drain() waiting for the reader to
release its active reference, resulting in a deadlock.

> +err_release_locks:
> +	release_update_locks_for_mdev(matrix_mdev);
>  	return ret;
>  }

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

Re: [PATCH v3 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:

Pre-existing issues:
- [High] Software resources (KVM GISC registration and pinned guest memory) are permanently leaked if hardware reset fails unexpectedly.
--

--- Patch [9]: [PATCH v3 9/9] s390/vfio-ap: Fix memory leak when queue removed from host AP config ---
commit b591517cf03c156d327043d4900cb31e47ff080b
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,
vfio_ap_mdev_remove_queue() can bypass resetting the queue. This patch fixes
a leak of KVM GISC resources and pinned guest memory by calling
vfio_ap_free_aqic_resources() if the adapter or domain is no longer in the
matrix configuration.

> diff --git a/drivers/s390/crypto/vfio_ap_ops.c b/drivers/s390/crypto/vfio_ap_ops.c
> index 3652ba792be73..faab5e9179ff1 100644
> --- a/drivers/s390/crypto/vfio_ap_ops.c
> +++ b/drivers/s390/crypto/vfio_ap_ops.c
> @@ -2507,12 +2507,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);

[Severity: High]
This is a pre-existing issue, but does vfio_ap_mdev_reset_queue() leak
software resources (KVM GISC registration and pinned guest memory) if 
hardware reset fails unexpectedly?

In vfio_ap_mdev_reset_queue(), if ap_zapq() returns an unexpected response
code, the default case is hit:

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

This default path returns without queuing the reset work or calling
vfio_ap_free_aqic_resources().

A similar situation occurs in the asynchronous worker thread apq_reset_check()
if ap_tapq() returns an unexpected response code:

		ret = apq_status_check(q->apqn, &status);
		if (ret == -EIO)
			return;

Since vfio_ap_mdev_remove_queue() continues on to call kfree(q) regardless of
the outcome of vfio_ap_mdev_reset_queue():

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

Are these resources permanently lost when this happens?

>  		flush_work(&q->reset_work);
> +	} else {
> +		vfio_ap_free_aqic_resources(q);
>  	}
>  
>  done:

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

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

[Christian Borntraeger]

Am 12.08.26 um 13:51 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 v2 => v3:
> ~~~~~~~~~~~~~~~~~~~
> Patch 3: Fix use of wrong lock in mdev probe function
> Patch 7: Fix required lock not held during display of sysfs status
> attribute
> * Squashed these two patches into a new patch entitled "Fix missing lock
>    required to access list of ap_matrix_mdev objects"
> * Added two new patches that fix pre-existing bugs:
>    ~ 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
> 
> 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 | 110 ++++++++++++++++++++----------
>   1 file changed, 75 insertions(+), 35 deletions(-)
> 


Sashiko has one new finding for patch8. Everything else is unrelated.
Will you do a respin for this?
I plan to take this for the next merge window.

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

[Matthew Rosato]

On 8/12/26 11:30 AM, Christian Borntraeger wrote:
> 
> Am 12.08.26 um 13:51 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 v2 => v3:
>> ~~~~~~~~~~~~~~~~~~~
>> Patch 3: Fix use of wrong lock in mdev probe function
>> Patch 7: Fix required lock not held during display of sysfs status
>> attribute
>> * Squashed these two patches into a new patch entitled "Fix missing lock
>>    required to access list of ap_matrix_mdev objects"
>> * Added two new patches that fix pre-existing bugs:
>>    ~ 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
>>
>> 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 | 110 ++++++++++++++++++++----------
>>   1 file changed, 75 insertions(+), 35 deletions(-)
>>
> 
> 
> Sashiko has one new finding for patch8. Everything else is unrelated.
> Will you do a respin for this?

I am going thru this version now but yes I would like a respin that
fixes patch 8.

> I plan to take this for the next merge window.

I am also going thru all of the Sashiko reports, most are already
resolved by this series.  But I will send Tony a pruned list to
investigate after this series vs adding more fixes to this one.

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

[Christian Borntraeger]

Am 12.08.26 um 17:36 schrieb Matthew Rosato:
> On 8/12/26 11:30 AM, Christian Borntraeger wrote:
>>
>> Am 12.08.26 um 13:51 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 v2 => v3:
>>> ~~~~~~~~~~~~~~~~~~~
>>> Patch 3: Fix use of wrong lock in mdev probe function
>>> Patch 7: Fix required lock not held during display of sysfs status
>>> attribute
>>> * Squashed these two patches into a new patch entitled "Fix missing lock
>>>     required to access list of ap_matrix_mdev objects"
>>> * Added two new patches that fix pre-existing bugs:
>>>     ~ 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
>>>
>>> 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 | 110 ++++++++++++++++++++----------
>>>    1 file changed, 75 insertions(+), 35 deletions(-)
>>>
>>
>>
>> Sashiko has one new finding for patch8. Everything else is unrelated.
>> Will you do a respin for this?
> 
> I am going thru this version now but yes I would like a respin that
> fixes patch 8.
> 
>> I plan to take this for the next merge window.
> 
> I am also going thru all of the Sashiko reports, most are already
> resolved by this series.  But I will send Tony a pruned list to
> investigate after this series vs adding more fixes to this one.

Let me know when this is ready. Apart from patch 8 I am happy.

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

[Matthew Rosato]

On 8/12/26 7:51 AM, Anthony Krowiak wrote:
> 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.
> 
> 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>

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

[Matthew Rosato]

On 8/12/26 7:51 AM, 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>

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

[Anthony Krowiak]

On 8/12/26 8:12 AM, sashiko-bot@kernel.org wrote:
> Thank you for your contribution! Sashiko AI review found 3 potential issue(s) to consider:
>
> Pre-existing issues:
> - [High] Uninitialized stack-allocated bitmap (`apm_filtered`) is passed to `reset_queues_for_apids`.
> - [High] Missing matrix_dev->mdevs_lock acquisition in vfio_ap_mdev_cfg_add() causing data race on matrix_mdev fields.
> - [High] Unsafe iteration over matrix_dev->mdev_list without holding mdevs_lock.
> --
>
> --- Patch [2]: [PATCH v3 2/9] s390/vfio-ap: Fix dereference matrix_mdev->kvm without checking for NULL ---
> commit adee11494c3d56eaa0ce36b3c479c7382d103cad
> Author: Anthony Krowiak <akrowiak@linux.ibm.com>
>
> s390/vfio-ap: Fix dereference matrix_mdev->kvm without checking for NULL
>
> This commit adds NULL checks for matrix_mdev->kvm before attempting to
> acquire the matrix_mdev->kvm->lock mutex in the configuration change and
> scan complete callbacks. This prevents a potential NULL pointer dereference
> if the kvm pointer has not been initialized.
>
>> --- a/drivers/s390/crypto/vfio_ap_ops.c
>> +++ b/drivers/s390/crypto/vfio_ap_ops.c
>> @@ -2606,7 +2606,13 @@ static void vfio_ap_mdev_cfg_remove(unsigned long *ap_remove,
>>   	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
> mdev_list holding only matrix_dev->guests_lock here?
>
> Looking at vfio_ap_mdev_probe(), it appears new entries are added to
> this list holding only matrix_dev->mdevs_lock (without taking guests_lock).
>
> Could concurrent probing during an AP bus scan modify the list while we are
> traversing it locklessly here, leading to list corruption?

Will add a block comment here to point out that it is safe to iterate 
the list
because vfio_ap_on_cfg_changed() - which is called prior to this - holds the
guests_lock. Hopefully that stops this from being pointed out by LLMs. Will
also add a comment prior to adding a matrix_mdev in the vfio_ap_mdev_probe
function for the same reason.

>
>> -		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);
> [Severity: High]
> This is a pre-existing issue, but does vfio_ap_mdev_cfg_add() also need
> matrix_dev->mdevs_lock protection like this?
>
> vfio_ap_mdev_cfg_add() is called right after vfio_ap_mdev_cfg_remove() from
> vfio_ap_on_cfg_changed(), which only holds matrix_dev->guests_lock. It
> modifies matrix_mdev->apm_add, aqm_add, and adm_add, and reads
> matrix_mdev->matrix.
>
> Could concurrent modifications race without mdevs_lock being held and
> corrupt the guest AP configuration bitmaps in vfio_ap_mdev_cfg_add()?

Fixed in patch 4/9 of this series.

>
> [ ... ]
>> @@ -2821,9 +2828,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 does apm_filtered need to be zeroed or
> initialized before use?
>
> If vfio_ap_on_scan_complete() is called with non-empty adm_add but empty
> apm_add and aqm_add, filter_adapters and filter_domains are set to false.
> This bypasses vfio_ap_mdev_filter_matrix(), which normally initializes
> apm_filtered.
>
> Then the uninitialized stack variable is passed down:
>
>      reset_queues_for_apids(matrix_mdev, apm_filtered);
>
> Could this cause reset_queues_for_apids() to iterate over random stack
> garbage bits and inadvertently reset arbitrary AP queues?

Fixed in patch 6/9 of this series.

>
> [ ... ]
>>   void vfio_ap_on_scan_complete(struct ap_config_info *new_config_info,
>> @@ -2854,15 +2855,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 isn't a bug introduced by this patch, but similar to the loop in
> vfio_ap_mdev_cfg_remove() above, does this iteration also need
> matrix_dev->mdevs_lock to prevent list corruption against concurrent calls
> to vfio_ap_mdev_probe()?

No, the matrix_dev->guests_lock is needed to prevent list corruption.

>
> [ ... ]
>

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

[Anthony Krowiak]

On 8/12/26 8:16 AM, sashiko-bot@kernel.org wrote:
> Thank you for your contribution! Sashiko AI review found 1 potential issue(s) to consider:
> - [High] ABBA deadlock in the probe error path between kernfs active reference and guests_lock.
> --
>
> --- Patch [8]: [PATCH v3 8/9] s390/vfio-ap: Fix NULL deref in status_show() during queue probe ---
> commit dd5ddcd73e238630c53e21354f9251a158e8bee2
> 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() during queue
> probe by acquiring update locks before creating the sysfs attribute group.
> This ensures any concurrent read blocks until driver data is set and locks
> are released.
>
>> diff --git a/drivers/s390/crypto/vfio_ap_ops.c b/drivers/s390/crypto/vfio_ap_ops.c
>> index cb1e5db69d8a7..3652ba792be73 100644
>> --- a/drivers/s390/crypto/vfio_ap_ops.c
>> +++ b/drivers/s390/crypto/vfio_ap_ops.c
>> @@ -2419,14 +2419,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) {
>> @@ -2434,11 +2437,10 @@ int vfio_ap_mdev_probe_queue(struct ap_device *apdev)
>>   		goto err_remove_group;
>>   	}
> [ ... ]
>> @@ -2468,6 +2470,8 @@ int vfio_ap_mdev_probe_queue(struct ap_device *apdev)
>>   
>>   err_remove_group:
>>   	sysfs_remove_group(&apdev->device.kobj, &vfio_queue_attr_group);
> [Severity: High]
> Does this create an ABBA deadlock on the error path?
>
> If kzalloc_obj() fails, we jump to err_remove_group and call
> sysfs_remove_group() while still holding matrix_dev->guests_lock (acquired
> via get_update_locks_by_apqn()).
>
> If a concurrent reader opens the 'status' sysfs file before we hit the error
> path, it acquires a kernfs active reference and blocks on:
>
> status_show()
>      mutex_lock(&matrix_dev->guests_lock);
>
> Then sysfs_remove_group() blocks in kernfs_drain() waiting for the reader to
> release its active reference, resulting in a deadlock.

release_update_locks_for_mdev() needs to be called before sysfs_remove_group

>
>> +err_release_locks:
>> +	release_update_locks_for_mdev(matrix_mdev);
>>   	return ret;
>>   }