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

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

[Anthony Krowiak]

Patches 2/5 and 5/5 in this series were previously posted as individual
fixes upstream. During review of those patches, the reviewer identified
several pre-existing problems in the same code paths that are addressed
by the remaining three patches. All five patches are fixes for
pre-existing bugs and are tagged for stable.

Patches 1/5, 3/5 and 4/5 fix bugs in the call trees of the two
callback functions implemented by the vfio_ap device
driver -- vfio_ap_on_cfg_changed and vfio_ap_on_scan_complete:

* Patch 1/5 fixes a stale do_remove flag in vfio_ap_mdev_cfg_remove
  that causes spurious hot-unplug operations on mdevs that have no
  assigned adapters, domains or control domains in common with those
  removed from the host AP configuration.

* Patch 3/5 fixes the use of the wrong lock to protect the addition of
  a newly created ap_matrix_mdev object to the mdev_list in the
  vfio_ap_mdev_probe function.

* Patch 4/5 fixes the missing mdevs_lock while updating the apm_add,
  aqm_add and adm_add fields of an ap_matrix_mdev object in
  vfio_ap_mdev_cfg_add.

Anthony Krowiak (5):
  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 use of wrong lock in mdev probe function
  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

 drivers/s390/crypto/vfio_ap_ops.c | 59 +++++++++++++++++++++----------
 1 file changed, 41 insertions(+), 18 deletions(-)

-- 
2.53.0

[PATCH 1/5] 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>
---
 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 2/5] 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>
---
 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 3/5] s390/vfio-ap: Fix use of wrong lock in mdev probe function

[Anthony Krowiak]

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 against traversal
of the list; 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 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.

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

diff --git a/drivers/s390/crypto/vfio_ap_ops.c b/drivers/s390/crypto/vfio_ap_ops.c
index e382e5a1cb99..a472de00bc14 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:
-- 
2.53.0

[PATCH 4/5] 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>
---
 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 a472de00bc14..897518b88c28 100644
--- a/drivers/s390/crypto/vfio_ap_ops.c
+++ b/drivers/s390/crypto/vfio_ap_ops.c
@@ -2756,12 +2756,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 5/5] 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>
---
 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 897518b88c28..9c7257c179a7 100644
--- a/drivers/s390/crypto/vfio_ap_ops.c
+++ b/drivers/s390/crypto/vfio_ap_ops.c
@@ -2621,9 +2621,9 @@ static void vfio_ap_mdev_cfg_remove(unsigned long *ap_remove,
 		do_remove |= bitmap_and(aqrem, aq_remove,
 					  matrix_mdev->matrix.aqm,
 					  AP_DOMAINS);
-		do_remove |= bitmap_andnot(cdrem, cd_remove,
-					     matrix_mdev->matrix.adm,
-					     AP_DOMAINS);
+		do_remove |= bitmap_and(cdrem, cd_remove,
+					matrix_mdev->matrix.adm,
+					AP_DOMAINS);
 
 		if (do_remove)
 			vfio_ap_mdev_hot_unplug_cfg(matrix_mdev, aprem, aqrem,
-- 
2.53.0

Re: [PATCH 5/5] 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:
- [Critical] Missing KVM guest configuration updates when the last AP device or domain is unplugged due to incorrect usage of `bitmap_andnot` return value.
--

--- Patch [5]: [PATCH 5/5] s390/vfio-ap: Fix control domain removal in vfio_ap_mdev_cfg_remove ---
commit c11c3bac302d3bc1f0a23db222df493383b0819f
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 (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.

> diff --git a/drivers/s390/crypto/vfio_ap_ops.c b/drivers/s390/crypto/vfio_ap_ops.c
> index 897518b88c281..9c7257c179a7a 100644
> --- a/drivers/s390/crypto/vfio_ap_ops.c
> +++ b/drivers/s390/crypto/vfio_ap_ops.c
> @@ -2621,9 +2621,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: Critical]
This is a pre-existing issue, but does the bitwise logic in
vfio_ap_mdev_hot_unplug_cfg() incorrectly assume bitmap_andnot() returns true
when bits are cleared?

Looking at vfio_ap_mdev_hot_unplug_cfg(), the return value of bitmap_andnot()
is used to determine if a hotplug update is necessary:

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

However, bitmap_andnot() returns false when the resulting destination bitmap
becomes empty.

If a host administrator unplugs the final active AP adapter or domain assigned
to an mdev, the unplugged bit is cleared from the guest mask, leaving the
shadow_apcb bitmap entirely empty. Because it is empty, bitmap_andnot()
returns false.

Will this cause do_hotplug to evaluate to 0 and skip calling
vfio_ap_mdev_update_guest_apcb() entirely, allowing the KVM guest to retain
hardware access to unplugged or revoked AP devices? The same pattern is used
for aqm and adm on subsequent lines.

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

Re: [PATCH 3/5] s390/vfio-ap: Fix use of wrong lock in mdev probe function

[sashiko-bot]

Thank you for your contribution! Sashiko AI review found 1 potential issue(s) to consider:
- [High] Data race and list corruption due to mismatched locks on list traversal and mutation.
--

--- Patch [3]: [PATCH 3/5] s390/vfio-ap: Fix use of wrong lock in mdev probe function ---
commit 6ed976a494c7af5de9033ac808583fad9a0cb4f7
Author: Anthony Krowiak <akrowiak@linux.ibm.com>

s390/vfio-ap: Fix use of wrong lock in mdev probe function

This commit changes the lock used to guard the addition of a newly created
ap_matrix_mdev object to matrix_dev->mdev_list in vfio_ap_mdev_probe()
from mdevs_lock to guests_lock.

> diff --git a/drivers/s390/crypto/vfio_ap_ops.c b/drivers/s390/crypto/vfio_ap_ops.c
> index e382e5a1cb996..a472de00bc149 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);

[Severity: High]
Does changing this lock to guests_lock create a data race when traversing
the list in other functions?

For example, when a userspace process reads the status attribute,
status_show() only acquires mdevs_lock before calling
vfio_ap_mdev_for_queue():

drivers/s390/crypto/vfio_ap_ops.c:status_show() {
    ...
	mutex_lock(&matrix_dev->mdevs_lock);
	q = dev_get_drvdata(&apdev->device);
	matrix_mdev = vfio_ap_mdev_for_queue(q);
    ...
}

Which then iterates over the list relying on the assumption that writers
will hold mdevs_lock:

drivers/s390/crypto/vfio_ap_ops.c:vfio_ap_mdev_for_queue() {
    ...
	list_for_each_entry(matrix_mdev, &matrix_dev->mdev_list, node) {
    ...
}

If vfio_ap_mdev_probe() is called concurrently with status_show(), could
this cause list corruption or a use-after-free panic since the reader and
writer are no longer synchronized with the same lock?

> +	mutex_unlock(&matrix_dev->guests_lock);
>  	return 0;
>  
>  err_put_vdev:

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

Re: [PATCH 4/5] 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/20260808170712.839066-1-akrowiak@linux.ibm.com?part=4

Re: [PATCH 2/5] 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 Memory Usage in `vfio_ap_mdev_hot_plug_cfg` leads to random hardware queue resets.
- [High] Use-After-Free in KVM's `pqap_hook` due to improper error rollback in `vfio_ap_mdev_set_kvm`.
- [High] Unsafe concurrent modification and iteration of `matrix_dev->mdev_list` leading to Data Race / UAF.
--

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

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

This commit resolves a potential NULL pointer dereference in the AP bus scan
callbacks by explicitly checking if matrix_mdev->kvm is set before acquiring
its lock. It also adjusts the lock acquisition order in these callbacks.

Cc: stable@vger.kernel.org

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

[ ... ]

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

[Severity: High]
This is a pre-existing issue, but does this function pass an uninitialized
bitmap to reset_queues_for_apids()?

If the AP configuration only adds control domains, filter_cdoms is true, but
both filter_adapters and filter_domains are false. This skips the call to
vfio_ap_mdev_filter_matrix() which initializes apm_filtered:

    if (filter_adapters || filter_domains)
        do_hotplug = vfio_ap_mdev_filter_matrix(matrix_mdev, apm_filtered);

If this initialization path is bypassed, is it possible for apm_filtered to
retain stack garbage when it is subsequently passed to
reset_queues_for_apids(), potentially causing unintended guest hardware
queues to be reset?

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

[Severity: High]
This is a pre-existing issue, but could this list iteration race with
concurrent list modifications?

It appears this list traversal in vfio_ap_on_scan_complete() occurs while
holding only matrix_dev->guests_lock at the list level. However, list
mutations such as the one in vfio_ap_mdev_probe():

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

are protected only by matrix_dev->mdevs_lock. Since the internal iteration
step happens outside of matrix_dev->mdevs_lock, can this expose the reader to
torn or stale pointers, potentially leading to a use-after-free or infinite
loop if an administrator concurrently creates a new mdev?

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

[ ... ]

While reviewing this file, I also noticed another potential issue outside the
immediate diff in vfio_ap_mdev_set_kvm():

>       kvm->arch.crypto.pqap_hook = &matrix_mdev->pqap_hook;
>       ...
>       if (m != matrix_mdev && m->kvm == kvm) {
>               release_update_locks_for_kvm(kvm);
>               return -EPERM;
>       }

[Severity: High]
This is a pre-existing issue, but is it possible for KVM's pqap_hook to be
left pointing to freed memory upon an error?

The KVM hook is unconditionally updated to point to the incoming matrix_mdev.
However, if another mdev is already attached to this KVM instance, the
function bails out with -EPERM.

If the rejected mdev is subsequently freed by userspace, would KVM retain a
dangling pointer? Could this cause a use-after-free when a guest later
issues a PQAP instruction and handle_pqap() dereferences the hook?

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

Re: [PATCH 1/5] 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:
- [Critical] Unconditional dereference of `matrix_mdev->kvm` pointer leading to host kernel panic.
- [High] Concurrent modification and traversal of `matrix_dev->mdev_list` without consistent locking.
- [High] Assigned control domains removed from the host are not unplugged from the KVM guest due to incorrect bitmap calculation.
- [High] Unplugging the last assigned AP resource from an mdev fails to update the KVM guest configuration.
--

--- Patch [1]: [PATCH 1/5] s390/vfio-ap: Fix stale do_remove flag across iterations in vfio_ap_mdev_cfg_remove ---
commit bb84df1768da7fbde04fb6494c22c41ed756bd46
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 an issue in vfio_ap_mdev_cfg_remove() where the do_remove
flag is incorrectly accumulated across iterations. It ensures the flag is
properly assigned on the first bitmap operation for each mdev.

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

[Severity: High]
This isn't a bug introduced by this patch, but does this code risk list
corruption or following invalid pointers?

The list is being iterated without holding mdevs_lock across the entire loop
(the lock is acquired and released inside the loop). Concurrently,
vfio_ap_mdev_probe() adds new mdevs to the list while holding only mdevs_lock:

drivers/s390/crypto/vfio_ap_ops.c:vfio_ap_mdev_probe() {
    ...
    list_add(&matrix_mdev->node, &matrix_dev->mdev_list);
    ...
}

Could this concurrent modification and traversal crash the host kernel?

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

[Severity: Critical]
This is a pre-existing issue, but can this unconditionally dereference a null
pointer?

When an mdev is created, its kvm pointer is null until it is attached to a
guest. If a host AP configuration change occurs while an unattached mdev
exists, this unconditionally executes mutex_lock(&matrix_mdev->kvm->lock),
which dereferences the null kvm pointer.

Could this lead to a host kernel panic?

>  		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 in vfio_ap_mdev_cfg_remove(), but is this bitmap
calculation correct?

Right below the quoted code, cdrem is calculated:

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

This evaluates to cd_remove & ~(matrix.adm), which yields domains removed from
the host that are explicitly not assigned to the mdev. Assigned domains that
are removed from the host are omitted and therefore not hot-unplugged.

Could this allow a control domain removed from the host AP configuration to
remain improperly assigned and accessible to the KVM guest?

[Severity: High]
This is another pre-existing issue, but does unplugging the last assigned
AP resource from an mdev fail to update the KVM guest configuration?

In vfio_ap_mdev_hot_unplug_cfg(), do_hotplug is determined using bitmap_andnot:

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

bitmap_andnot returns true only if the resulting destination bitmap is
non-empty. If the last adapter or domain is removed, the resulting mask is
empty, bitmap_andnot returns false, and do_hotplug remains false.

Would this skip the APCB update, leaving the KVM guest with its old KVM CRYCBD
configuration and retaining access to an AP resource that was successfully
removed from the host?

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

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

[Anthony Krowiak]

On 8/8/26 1:49 PM, sashiko-bot@kernel.org wrote:
> Thank you for your contribution! Sashiko AI review found 4 potential issue(s) to consider:
>
> Pre-existing issues:
> - [Critical] Unconditional dereference of `matrix_mdev->kvm` pointer leading to host kernel panic.
> - [High] Concurrent modification and traversal of `matrix_dev->mdev_list` without consistent locking.
> - [High] Assigned control domains removed from the host are not unplugged from the KVM guest due to incorrect bitmap calculation.
> - [High] Unplugging the last assigned AP resource from an mdev fails to update the KVM guest configuration.
> --
>
> --- Patch [1]: [PATCH 1/5] s390/vfio-ap: Fix stale do_remove flag across iterations in vfio_ap_mdev_cfg_remove ---
> commit bb84df1768da7fbde04fb6494c22c41ed756bd46
> 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 an issue in vfio_ap_mdev_cfg_remove() where the do_remove
> flag is incorrectly accumulated across iterations. It ensures the flag is
> properly assigned on the first bitmap operation for each mdev.
>
>> diff --git a/drivers/s390/crypto/vfio_ap_ops.c b/drivers/s390/crypto/vfio_ap_ops.c
>> index 44b3a1dcc1b36..845c86ba8bc30 100644
>> --- a/drivers/s390/crypto/vfio_ap_ops.c
>> +++ b/drivers/s390/crypto/vfio_ap_ops.c
>> @@ -2603,15 +2603,15 @@ static void vfio_ap_mdev_cfg_remove(unsigned long *ap_remove,
>>   	DECLARE_BITMAP(aprem, AP_DEVICES);
>>   	DECLARE_BITMAP(aqrem, AP_DOMAINS);
>>   	DECLARE_BITMAP(cdrem, AP_DOMAINS);
>> -	int do_remove = 0;
>> +	int do_remove;
>>   
>>   	list_for_each_entry(matrix_mdev, &matrix_dev->mdev_list, node) {
> [Severity: High]
> This isn't a bug introduced by this patch, but does this code risk list
> corruption or following invalid pointers?
>
> The list is being iterated without holding mdevs_lock across the entire loop
> (the lock is acquired and released inside the loop). Concurrently,
> vfio_ap_mdev_probe() adds new mdevs to the list while holding only mdevs_lock:
>
> drivers/s390/crypto/vfio_ap_ops.c:vfio_ap_mdev_probe() {
>      ...
>      list_add(&matrix_mdev->node, &matrix_dev->mdev_list);
>      ...
> }
>
> Could this concurrent modification and traversal crash the host kernel?

No. The call stack for this function is:
vfio_ap_on_cfg_changed -> vfio_ap_mdev_on_cfg_remove -> 
vfio_ap_mdev_config_remove

In vfio_ap_cfg_changed, the matrix_dev->guests_lock mutex is taken and 
held for the duration
of the function. The guests_lock guards access to the 
matrix_dev->mdev_list. All functions
that add or remove ap_matrix_mdev objects also take this mutex, so there 
should never be
concurrent modification of the list.

>
>>   		mutex_lock(&matrix_mdev->kvm->lock);
> [Severity: Critical]
> This is a pre-existing issue, but can this unconditionally dereference a null
> pointer?

It can; however, the problem is fixed with patch 02/05 in the patch 
series in which this patch is
included.

>
> When an mdev is created, its kvm pointer is null until it is attached to a
> guest. If a host AP configuration change occurs while an unattached mdev
> exists, this unconditionally executes mutex_lock(&matrix_mdev->kvm->lock),
> which dereferences the null kvm pointer.
>
> Could this lead to a host kernel panic?
>
>>   		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 in vfio_ap_mdev_cfg_remove(), but is this bitmap
> calculation correct?
>
> Right below the quoted code, cdrem is calculated:
>
> drivers/s390/crypto/vfio_ap_ops.c:vfio_ap_mdev_cfg_remove() {
>      ...
> 		do_remove |= bitmap_andnot(cdrem, cd_remove,
> 					     matrix_mdev->matrix.adm,
> 					     AP_DOMAINS);
>      ...
> }
>
> This evaluates to cd_remove & ~(matrix.adm), which yields domains removed from
> the host that are explicitly not assigned to the mdev. Assigned domains that
> are removed from the host are omitted and therefore not hot-unplugged.
>
> Could this allow a control domain removed from the host AP configuration to
> remain improperly assigned and accessible to the KVM guest?

This problem is fixed with patch 5/5 in the patch series in which this 
patch is
included.

>
> [Severity: High]
> This is another pre-existing issue, but does unplugging the last assigned
> AP resource from an mdev fail to update the KVM guest configuration?
>
> In vfio_ap_mdev_hot_unplug_cfg(), do_hotplug is determined using bitmap_andnot:
>
> 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);
> 	}
>      ...
> }
>
> bitmap_andnot returns true only if the resulting destination bitmap is
> non-empty. If the last adapter or domain is removed, the resulting mask is
> empty, bitmap_andnot returns false, and do_hotplug remains false.
>
> Would this skip the APCB update, leaving the KVM guest with its old KVM CRYCBD
> configuration and retaining access to an AP resource that was successfully
> removed from the host?

The logic here is correct. aprem is a bitmap specifying the adapters 
that have
been removed from the host's AP configuration. The bitmap_andnot will return
true only if the matrix_mdev->shadow_apcb and aprem and therefore the
intersecting bits have been removed from shadow_apcb. If there is not
intersection, then no bits will have been removed and the bitmap_andnot
will return 0, in which case there is no need to make changes on the guest.

>

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

[Anthony Krowiak]

On 8/8/26 1:36 PM, 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 Memory Usage in `vfio_ap_mdev_hot_plug_cfg` leads to random hardware queue resets.
> - [High] Use-After-Free in KVM's `pqap_hook` due to improper error rollback in `vfio_ap_mdev_set_kvm`.
> - [High] Unsafe concurrent modification and iteration of `matrix_dev->mdev_list` leading to Data Race / UAF.
> --
>
> --- Patch [2]: [PATCH 2/5] s390/vfio-ap: Fix dereference matrix_mdev->kvm without checking for NULL ---
> commit 9e16afa935b6610f6431f2a073e3d436ec0b7f7d
> Author: Anthony Krowiak <akrowiak@linux.ibm.com>
>
> s390/vfio-ap: Fix dereference matrix_mdev->kvm without checking for NULL
>
> This commit resolves a potential NULL pointer dereference in the AP bus scan
> callbacks by explicitly checking if matrix_mdev->kvm is set before acquiring
> its lock. It also adjusts the lock acquisition order in these callbacks.
>
> Cc: stable@vger.kernel.org
>
>> diff --git a/drivers/s390/crypto/vfio_ap_ops.c b/drivers/s390/crypto/vfio_ap_ops.c
>> index 845c86ba8bc30..e382e5a1cb996 100644
>> --- a/drivers/s390/crypto/vfio_ap_ops.c
>> +++ b/drivers/s390/crypto/vfio_ap_ops.c
> [ ... ]
>
>> @@ -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);
>> https://lkml.org/lkml/2026/8/6/1775
> [Severity: High]
> This is a pre-existing issue, but does this function pass an uninitialized
> bitmap to reset_queues_for_apids()?
>
> If the AP configuration only adds control domains, filter_cdoms is true, but
> both filter_adapters and filter_domains are false. This skips the call to
> vfio_ap_mdev_filter_matrix() which initializes apm_filtered:
>
>      if (filter_adapters || filter_domains)
>          do_hotplug = vfio_ap_mdev_filter_matrix(matrix_mdev, apm_filtered);
>
> If this initialization path is bypassed, is it possible for apm_filtered to
> retain stack garbage when it is subsequently passed to
> reset_queues_for_apids(), potentially causing unintended guest hardware
> queues to be reset?

Yes, because if no adapters or domains need to be filtered, the call to
vfio_ap_mdev_filter_matrix - which zeros all bits to apm_filtered - will 
be bypassed.
The apm_filtered bitmap needs to be zeroed before the check for 
filter_adapters
and filter_domains.

>
>> -
>> -	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) {
> [Severity: High]
> This is a pre-existing issue, but could this list iteration race with
> concurrent list modifications?
>
> It appears this list traversal in vfio_ap_on_scan_complete() occurs while
> holding only matrix_dev->guests_lock at the list level. However, list
> mutations such as the one in vfio_ap_mdev_probe():
>
> 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);
>      ...
> }
>
> are protected only by matrix_dev->mdevs_lock. Since the internal iteration
> step happens outside of matrix_dev->mdevs_lock, can this expose the reader to
> torn or stale pointers, potentially leading to a use-after-free or infinite
> loop if an administrator concurrently creates a new mdev?

This is fixed via patch 3/5 of the series in which this patch is included.

>
>> +		/*
>> +		 * 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.
>> +		 */
> [ ... ]
>
> While reviewing this file, I also noticed another potential issue outside the
> immediate diff in vfio_ap_mdev_set_kvm():
>
>>        kvm->arch.crypto.pqap_hook = &matrix_mdev->pqap_hook;
>>        ...
>>        if (m != matrix_mdev && m->kvm == kvm) {
>>                release_update_locks_for_kvm(kvm);
>>                return -EPERM;
>>        }
> [Severity: High]
> This is a pre-existing issue, but is it possible for KVM's pqap_hook to be
> left pointing to freed memory upon an error?
>
> The KVM hook is unconditionally updated to point to the incoming matrix_mdev.
> However, if another mdev is already attached to this KVM instance, the
> function bails out with -EPERM.
>
> If the rejected mdev is subsequently freed by userspace, would KVM retain a
> dangling pointer? Could this cause a use-after-free when a guest later
> issues a PQAP instruction and handle_pqap() dereferences the hook?

A fix for this has already been submitted to the kernel mailing list:
https://lkml.org/lkml/2026/8/6/1775

>

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

[Anthony Krowiak]

On 8/8/26 1:30 PM, sashiko-bot@kernel.org wrote:
> Thank you for your contribution! Sashiko AI review found 1 potential issue(s) to consider:
>
> Pre-existing issues:
> - [Critical] Missing KVM guest configuration updates when the last AP device or domain is unplugged due to incorrect usage of `bitmap_andnot` return value.
> --
>
> --- Patch [5]: [PATCH 5/5] s390/vfio-ap: Fix control domain removal in vfio_ap_mdev_cfg_remove ---
> commit c11c3bac302d3bc1f0a23db222df493383b0819f
> 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 (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.
>
>> diff --git a/drivers/s390/crypto/vfio_ap_ops.c b/drivers/s390/crypto/vfio_ap_ops.c
>> index 897518b88c281..9c7257c179a7a 100644
>> --- a/drivers/s390/crypto/vfio_ap_ops.c
>> +++ b/drivers/s390/crypto/vfio_ap_ops.c
>> @@ -2621,9 +2621,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: Critical]
> This is a pre-existing issue, but does the bitwise logic in
> vfio_ap_mdev_hot_unplug_cfg() incorrectly assume bitmap_andnot() returns true
> when bits are cleared?
>
> Looking at vfio_ap_mdev_hot_unplug_cfg(), the return value of bitmap_andnot()
> is used to determine if a hotplug update is necessary:
>
> 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);
> }
>
> However, bitmap_andnot() returns false when the resulting destination bitmap
> becomes empty.
>
> If a host administrator unplugs the final active AP adapter or domain assigned
> to an mdev, the unplugged bit is cleared from the guest mask, leaving the
> shadow_apcb bitmap entirely empty. Because it is empty, bitmap_andnot()
> returns false.
>
> Will this cause do_hotplug to evaluate to 0 and skip calling
> vfio_ap_mdev_update_guest_apcb() entirely, allowing the KVM guest to retain
> hardware access to unplugged or revoked AP devices? The same pattern is used
> for aqm and adm on subsequent lines.

This is a bug. The author obviously mistook the value returned from 
bitmap_andnot.
The fix will be to check whether the input bitmaps intersect with the 
corresponding
bitmaps in matrix_mdev->shadow_apm/aqm/adm and then do the bitmap_and
only if they intersect and set do_hotplug to true.

>

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

[Anthony Krowiak]

On 8/10/26 10:01 AM, Anthony Krowiak wrote:
>
>
> On 8/8/26 1:49 PM, sashiko-bot@kernel.org wrote:
>> Thank you for your contribution! Sashiko AI review found 4 potential 
>> issue(s) to consider:
>>
>> Pre-existing issues:
>> - [Critical] Unconditional dereference of `matrix_mdev->kvm` pointer 
>> leading to host kernel panic.
>> - [High] Concurrent modification and traversal of 
>> `matrix_dev->mdev_list` without consistent locking.
>> - [High] Assigned control domains removed from the host are not 
>> unplugged from the KVM guest due to incorrect bitmap calculation.
>> - [High] Unplugging the last assigned AP resource from an mdev fails 
>> to update the KVM guest configuration.
>> -- 
>>
>> --- Patch [1]: [PATCH 1/5] s390/vfio-ap: Fix stale do_remove flag 
>> across iterations in vfio_ap_mdev_cfg_remove ---
>> commit bb84df1768da7fbde04fb6494c22c41ed756bd46
>> 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 an issue in vfio_ap_mdev_cfg_remove() where the 
>> do_remove
>> flag is incorrectly accumulated across iterations. It ensures the 
>> flag is
>> properly assigned on the first bitmap operation for each mdev.
>>
>>> diff --git a/drivers/s390/crypto/vfio_ap_ops.c 
>>> b/drivers/s390/crypto/vfio_ap_ops.c
>>> index 44b3a1dcc1b36..845c86ba8bc30 100644
>>> --- a/drivers/s390/crypto/vfio_ap_ops.c
>>> +++ b/drivers/s390/crypto/vfio_ap_ops.c
>>> @@ -2603,15 +2603,15 @@ static void vfio_ap_mdev_cfg_remove(unsigned 
>>> long *ap_remove,
>>>       DECLARE_BITMAP(aprem, AP_DEVICES);
>>>       DECLARE_BITMAP(aqrem, AP_DOMAINS);
>>>       DECLARE_BITMAP(cdrem, AP_DOMAINS);
>>> -    int do_remove = 0;
>>> +    int do_remove;
>>>         list_for_each_entry(matrix_mdev, &matrix_dev->mdev_list, 
>>> node) {
>> [Severity: High]
>> This isn't a bug introduced by this patch, but does this code risk list
>> corruption or following invalid pointers?
>>
>> The list is being iterated without holding mdevs_lock across the 
>> entire loop
>> (the lock is acquired and released inside the loop). Concurrently,
>> vfio_ap_mdev_probe() adds new mdevs to the list while holding only 
>> mdevs_lock:
>>
>> drivers/s390/crypto/vfio_ap_ops.c:vfio_ap_mdev_probe() {
>>      ...
>>      list_add(&matrix_mdev->node, &matrix_dev->mdev_list);
>>      ...
>> }
>>
>> Could this concurrent modification and traversal crash the host kernel?
>
> No. The call stack for this function is:
> vfio_ap_on_cfg_changed -> vfio_ap_mdev_on_cfg_remove -> 
> vfio_ap_mdev_config_remove
>
> In vfio_ap_cfg_changed, the matrix_dev->guests_lock mutex is taken and 
> held for the duration
> of the function. The guests_lock guards access to the 
> matrix_dev->mdev_list. All functions
> that add or remove ap_matrix_mdev objects also take this mutex, so 
> there should never be
> concurrent modification of the list.
>
>>
>>> mutex_lock(&matrix_mdev->kvm->lock);
>> [Severity: Critical]
>> This is a pre-existing issue, but can this unconditionally 
>> dereference a null
>> pointer?
>
> It can; however, the problem is fixed with patch 02/05 in the patch 
> series in which this patch is
> included.
>
>>
>> When an mdev is created, its kvm pointer is null until it is attached 
>> to a
>> guest. If a host AP configuration change occurs while an unattached mdev
>> exists, this unconditionally executes 
>> mutex_lock(&matrix_mdev->kvm->lock),
>> which dereferences the null kvm pointer.
>>
>> Could this lead to a host kernel panic?
>>
>>> 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 in vfio_ap_mdev_cfg_remove(), but is 
>> this bitmap
>> calculation correct?
>>
>> Right below the quoted code, cdrem is calculated:
>>
>> drivers/s390/crypto/vfio_ap_ops.c:vfio_ap_mdev_cfg_remove() {
>>      ...
>>         do_remove |= bitmap_andnot(cdrem, cd_remove,
>>                          matrix_mdev->matrix.adm,
>>                          AP_DOMAINS);
>>      ...
>> }
>>
>> This evaluates to cd_remove & ~(matrix.adm), which yields domains 
>> removed from
>> the host that are explicitly not assigned to the mdev. Assigned 
>> domains that
>> are removed from the host are omitted and therefore not hot-unplugged.
>>
>> Could this allow a control domain removed from the host AP 
>> configuration to
>> remain improperly assigned and accessible to the KVM guest?
>
> This problem is fixed with patch 5/5 in the patch series in which this 
> patch is
> included.
>
>>
>> [Severity: High]
>> This is another pre-existing issue, but does unplugging the last 
>> assigned
>> AP resource from an mdev fail to update the KVM guest configuration?
>>
>> In vfio_ap_mdev_hot_unplug_cfg(), do_hotplug is determined using 
>> bitmap_andnot:
>>
>> 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);
>>     }
>>      ...
>> }
>>
>> bitmap_andnot returns true only if the resulting destination bitmap is
>> non-empty. If the last adapter or domain is removed, the resulting 
>> mask is
>> empty, bitmap_andnot returns false, and do_hotplug remains false.
>>
>> Would this skip the APCB update, leaving the KVM guest with its old 
>> KVM CRYCBD
>> configuration and retaining access to an AP resource that was 
>> successfully
>> removed from the host?
>
> The logic here is correct. aprem is a bitmap specifying the adapters 
> that have
> been removed from the host's AP configuration. The bitmap_andnot will 
> return
> true only if the matrix_mdev->shadow_apcb and aprem and therefore the
> intersecting bits have been removed from shadow_apcb. If there is not
> intersection, then no bits will have been removed and the bitmap_andnot
> will return 0, in which case there is no need to make changes on the 
> guest.

I am wrong here; a misunderstanding of the return value of 
bitmap_andnot. See my
response to patch 5/5 which mentions this same thing.


>
>>
>