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

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

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 44b3a1dcc1b3..a0b7c37fee28 100644
--- a/drivers/s390/crypto/vfio_ap_ops.c
+++ b/drivers/s390/crypto/vfio_ap_ops.c
@@ -2606,7 +2606,15 @@ static void vfio_ap_mdev_cfg_remove(unsigned long *ap_remove,
 	int do_remove = 0;
 
 	list_for_each_entry(matrix_mdev, &matrix_dev->mdev_list, node) {
-		mutex_lock(&matrix_mdev->kvm->lock);
+		/*
+		 * If the mdev is attached to a KVM guest, we will need to
+		 * hold the KVM lock in order to update the guest's AP
+		 * configuration if any adapters, domains or control domains
+		 * have been removed.
+		 */
+		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 +2632,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);
 	}
 }
 
@@ -2748,6 +2757,7 @@ 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);
 
+	mutex_lock(&matrix_dev->mdevs_lock);
 	list_for_each_entry(matrix_mdev, &matrix_dev->mdev_list, node) {
 		bitmap_and(matrix_mdev->apm_add,
 			   matrix_mdev->matrix.apm, apm_add, AP_DEVICES);
@@ -2756,6 +2766,7 @@ static void vfio_ap_mdev_cfg_add(unsigned long *apm_add, unsigned long *aqm_add,
 		bitmap_and(matrix_mdev->adm_add,
 			   matrix_mdev->matrix.adm, adm_add, AP_DEVICES);
 	}
+	mutex_unlock(&matrix_dev->mdevs_lock);
 }
 
 /**
@@ -2821,9 +2832,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 +2849,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 +2859,25 @@ 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) {
+		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 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);
+
+unlock:
+		mutex_unlock(&matrix_dev->mdevs_lock);
+		if (matrix_mdev->kvm)
+			mutex_unlock(&matrix_mdev->kvm->lock);
 	}
 
 	mutex_unlock(&matrix_dev->guests_lock);
-- 
2.53.0

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

[sashiko-bot]

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

Pre-existing issues:
- [High] Incorrect bitwise operation `bitmap_andnot` used instead of `bitmap_and` for control domains in `vfio_ap_mdev_cfg_remove`, preventing removal of unplugged domains.
--

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

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

This commit adds NULL pointer checks for matrix_mdev->kvm prior to taking
the kvm->lock mutex in the configuration change and scan complete callbacks.
This prevents a NULL pointer dereference when the matrix mdev has no
associated KVM instance.

Cc: stable@vger.kernel.org

> diff --git a/drivers/s390/crypto/vfio_ap_ops.c b/drivers/s390/crypto/vfio_ap_ops.c
> index 44b3a1dcc1b36..a0b7c37fee281 100644
> --- a/drivers/s390/crypto/vfio_ap_ops.c
> +++ b/drivers/s390/crypto/vfio_ap_ops.c
> @@ -2606,7 +2606,15 @@ static void vfio_ap_mdev_cfg_remove(unsigned long *ap_remove,
>  	int do_remove = 0;
>  
>  	list_for_each_entry(matrix_mdev, &matrix_dev->mdev_list, node) {
> -		mutex_lock(&matrix_mdev->kvm->lock);
> +		/*
> +		 * If the mdev is attached to a KVM guest, we will need to
> +		 * hold the KVM lock in order to update the guest's AP
> +		 * configuration if any adapters, domains or control domains
> +		 * have been removed.
> +		 */
> +		if (matrix_mdev->kvm)
> +			mutex_lock(&matrix_mdev->kvm->lock);
> +
>  		mutex_lock(&matrix_dev->mdevs_lock);
>  
>  		do_remove |= bitmap_and(aprem, ap_remove,

[Severity: High]
This is a pre-existing issue, but I noticed a potential logic flaw just
below this context in vfio_ap_mdev_cfg_remove() within
drivers/s390/crypto/vfio_ap_ops.c.

Does the control domain removal incorrectly use bitmap_andnot() instead of
bitmap_and()?

When a host administrator or hypervisor unplugs a control domain, the code
evaluates the removed domains using:

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

Since this evaluates to cd_remove & ~matrix_mdev->matrix.adm, doesn't this
mean assigned domains that were removed from the host will resolve to 0?

Could this prevent explicitly unplugged control domains from being removed
from the KVM guest, allowing it to retain access to hardware it shouldn't
have?

[ ... ]

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

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

[Anthony Krowiak]

On 8/7/26 11:18 AM, sashiko-bot@kernel.org wrote:
> Thank you for your contribution! Sashiko AI review found 1 potential issue(s) to consider:
>
> Pre-existing issues:
> - [High] Incorrect bitwise operation `bitmap_andnot` used instead of `bitmap_and` for control domains in `vfio_ap_mdev_cfg_remove`, preventing removal of unplugged domains.
> --
>
> --- Patch [1]: [PATCH] s390/vfio-ap: Fix dereference matrix_mdev->kvm without checking for NULL ---
> commit 7ae73def648a068715a8cdb4bf574ac5755f8e77
> Author: Anthony Krowiak <akrowiak@linux.ibm.com>
>
> s390/vfio-ap: Fix dereference matrix_mdev->kvm without checking for NULL
>
> This commit adds NULL pointer checks for matrix_mdev->kvm prior to taking
> the kvm->lock mutex in the configuration change and scan complete callbacks.
> This prevents a NULL pointer dereference when the matrix mdev has no
> associated KVM instance.
>
> Cc: stable@vger.kernel.org
>
>> diff --git a/drivers/s390/crypto/vfio_ap_ops.c b/drivers/s390/crypto/vfio_ap_ops.c
>> index 44b3a1dcc1b36..a0b7c37fee281 100644
>> --- a/drivers/s390/crypto/vfio_ap_ops.c
>> +++ b/drivers/s390/crypto/vfio_ap_ops.c
>> @@ -2606,7 +2606,15 @@ static void vfio_ap_mdev_cfg_remove(unsigned long *ap_remove,
>>   	int do_remove = 0;
>>   
>>   	list_for_each_entry(matrix_mdev, &matrix_dev->mdev_list, node) {
>> -		mutex_lock(&matrix_mdev->kvm->lock);
>> +		/*
>> +		 * If the mdev is attached to a KVM guest, we will need to
>> +		 * hold the KVM lock in order to update the guest's AP
>> +		 * configuration if any adapters, domains or control domains
>> +		 * have been removed.
>> +		 */
>> +		if (matrix_mdev->kvm)
>> +			mutex_lock(&matrix_mdev->kvm->lock);
>> +
>>   		mutex_lock(&matrix_dev->mdevs_lock);
>>   
>>   		do_remove |= bitmap_and(aprem, ap_remove,
> [Severity: High]
> This is a pre-existing issue, but I noticed a potential logic flaw just
> below this context in vfio_ap_mdev_cfg_remove() within
> drivers/s390/crypto/vfio_ap_ops.c.
>
> Does the control domain removal incorrectly use bitmap_andnot() instead of
> bitmap_and()?
>
> When a host administrator or hypervisor unplugs a control domain, the code
> evaluates the removed domains using:
>
> 	do_remove |= bitmap_andnot(cdrem, cd_remove,
> 				   matrix_mdev->matrix.adm,
> 				   AP_DOMAINS);
>
> Since this evaluates to cd_remove & ~matrix_mdev->matrix.adm, doesn't this
> mean assigned domains that were removed from the host will resolve to 0?
>
> Could this prevent explicitly unplugged control domains from being removed
> from the KVM guest, allowing it to retain access to hardware it shouldn't
> have?

This looks like a cut and paste error from long ago. bitmap_and should have
been used as it was for the other bitmaps instead of bitmap_andnot.

>
> [ ... ]
>