Linux s390 Architecture development
 help / color / mirror / Atom feed
* [PATCH v4] s390/vfio-ap: fix stale pqap_hook pointer on error in vfio_ap_mdev_set_kvm()
@ 2026-08-06 14:03 Anthony Krowiak
  2026-08-06 14:18 ` sashiko-bot
                   ` (2 more replies)
  0 siblings, 3 replies; 6+ messages in thread
From: Anthony Krowiak @ 2026-08-06 14:03 UTC (permalink / raw)
  To: linux-s390, linux-kernel, kvm
  Cc: jjherne, borntraeger, mjrosato, pasic, alex, kwankhede, hca, gor,
	agordeev, stable

In vfio_ap_mdev_set_kvm(), kvm->arch.crypto.pqap_hook is set to
&matrix_mdev->pqap_hook before the update locks are acquired and the
mdev list is checked for a conflicting assignment. If another mdev is
already attached to the same KVM instance, the function returns -EPERM
without restoring the hook pointer, leaving kvm->arch.crypto.pqap_hook
pointing at the failing matrix_mdev instead of the mdev that legitimately
owns the KVM.

Since matrix_mdev->kvm is never set on this error path,
vfio_ap_mdev_unset_kvm() will not clean up the hook when matrix_mdev
is later closed. If matrix_mdev is subsequently freed, any PQAP
instruction executed by the guest will dereference the stale pointer
through pqap_hook_rwsem, resulting in a use-after-free.

Since kvm->arch.crypto.pqap_hook is only set in the vfio_ap_mdev_set_kvm()
function and is cleared in the vfio_ap_mdev_unset_kvm() function, a check
for 'kvm->arch.crypto.pqap_hook != NULL' is all that is needed to determine
whether it belongs to another mdev. This will alleviate the need to iterate
the matrix_dev->mdev_list list to see if the kvm object is assigned to
another mdev.This was introduced in v3 to alleviate the need to take the
mdevs_lock while iterating the list; however, this did not prevent a
potential race condition.

The pqap_hook_rwsem(write) is now performed inside
get_update_locks_for_kvm(), which is updated to acquire
pqap_hook_rwsem(write) between kvm->lock and mdevs_lock. This ordering
is consistent with the PQAP intercept path, which acquires pqap_hook_rwsem
in read mode while srcu is held under vcpu->mutex, establishing the
dependency: kvm->lock -> vcpu->mutex -> srcu -> pqap_hook_rwsem(read).

The pqap_hook_rwsem is now released inside the
release_update_locks_for_kvm(), which is updated to release
pqap_hook_rwsem(write) between mdevs_lock and kvm->lock.

Additionally, kvm_put_kvm() in vfio_ap_mdev_unset_kvm() is moved
after release_update_locks_for_kvm(). Previously it was called while
kvm->lock was held; if it were ever the last reference, kvm_destroy_vm()
would run under kvm->lock, which would deadlock.

Fixes: 86956e70761b3 ("s390/vfio-ap: replace open coded locks for VFIO_GROUP_NOTIFY_SET_KVM notification")
Cc: stable@vger.kernel.org
Signed-off-by: Anthony Krowiak <akrowiak@linux.ibm.com>
Signed-off-by: Matthew Rosato <mjrosato@linux.ibm.com>
---
 drivers/s390/crypto/vfio_ap_ops.c | 45 ++++++++++++++-----------------
 1 file changed, 20 insertions(+), 25 deletions(-)

diff --git a/drivers/s390/crypto/vfio_ap_ops.c b/drivers/s390/crypto/vfio_ap_ops.c
index 44b3a1dcc1b3..99a0efd999ef 100644
--- a/drivers/s390/crypto/vfio_ap_ops.c
+++ b/drivers/s390/crypto/vfio_ap_ops.c
@@ -48,15 +48,19 @@ static void vfio_ap_mdev_reset_queue(struct vfio_ap_queue *q);
  * 1. matrix_dev->guests_lock: required to use the KVM pointer to update a KVM
  *			       guest's APCB.
  * 2. kvm->lock:	       required to update a guest's APCB
- * 3. matrix_dev->mdevs_lock:  required to access data stored in a matrix_mdev
+ * 3. kvm->arch.crypto.pqap_hook_rwsem: required to update pqap_hook and
+ *					serialize against PQAP intercepts
+ * 4. matrix_dev->mdevs_lock:  required to access data stored in a matrix_mdev
  *
- * Note: If @kvm is NULL, the KVM lock will not be taken.
+ * Note: If @kvm is NULL, the KVM lock and pqap_hook_rwsem will not be taken.
  */
 static inline void get_update_locks_for_kvm(struct kvm *kvm)
 {
 	mutex_lock(&matrix_dev->guests_lock);
-	if (kvm)
+	if (kvm) {
 		mutex_lock(&kvm->lock);
+		down_write(&kvm->arch.crypto.pqap_hook_rwsem);
+	}
 	mutex_lock(&matrix_dev->mdevs_lock);
 }
 
@@ -68,16 +72,19 @@ static inline void get_update_locks_for_kvm(struct kvm *kvm)
  *
  * The proper unlocking order is:
  * 1. matrix_dev->mdevs_lock
- * 2. kvm->lock
- * 3. matrix_dev->guests_lock
+ * 2. kvm->arch.crypto.pqap_hook_rwsem
+ * 3. kvm->lock
+ * 4. matrix_dev->guests_lock
  *
- * Note: If @kvm is NULL, the KVM lock will not be released.
+ * Note: If @kvm is NULL, the KVM lock and pqap_hook_rwsem will not be released.
  */
 static inline void release_update_locks_for_kvm(struct kvm *kvm)
 {
 	mutex_unlock(&matrix_dev->mdevs_lock);
-	if (kvm)
+	if (kvm) {
+		up_write(&kvm->arch.crypto.pqap_hook_rwsem);
 		mutex_unlock(&kvm->lock);
+	}
 	mutex_unlock(&matrix_dev->guests_lock);
 }
 
@@ -1821,26 +1828,17 @@ static const struct attribute_group *vfio_ap_mdev_attr_groups[] = {
 static int vfio_ap_mdev_set_kvm(struct ap_matrix_mdev *matrix_mdev,
 				struct kvm *kvm)
 {
-	struct ap_matrix_mdev *m;
-
 	if (kvm->arch.crypto.crycbd) {
-		down_write(&kvm->arch.crypto.pqap_hook_rwsem);
-		kvm->arch.crypto.pqap_hook = &matrix_mdev->pqap_hook;
-		up_write(&kvm->arch.crypto.pqap_hook_rwsem);
-
 		get_update_locks_for_kvm(kvm);
-
-		list_for_each_entry(m, &matrix_dev->mdev_list, node) {
-			if (m != matrix_mdev && m->kvm == kvm) {
-				release_update_locks_for_kvm(kvm);
-				return -EPERM;
-			}
+		if (kvm->arch.crypto.pqap_hook) {
+			release_update_locks_for_kvm(kvm);
+			return -EPERM;
 		}
+		kvm->arch.crypto.pqap_hook = &matrix_mdev->pqap_hook;
 
 		kvm_get_kvm(kvm);
 		matrix_mdev->kvm = kvm;
 		vfio_ap_mdev_update_guest_apcb(matrix_mdev);
-
 		release_update_locks_for_kvm(kvm);
 	}
 
@@ -1883,18 +1881,15 @@ static void vfio_ap_mdev_unset_kvm(struct ap_matrix_mdev *matrix_mdev)
 	struct kvm *kvm = matrix_mdev->kvm;
 
 	if (kvm && kvm->arch.crypto.crycbd) {
-		down_write(&kvm->arch.crypto.pqap_hook_rwsem);
-		kvm->arch.crypto.pqap_hook = NULL;
-		up_write(&kvm->arch.crypto.pqap_hook_rwsem);
-
 		get_update_locks_for_kvm(kvm);
+		kvm->arch.crypto.pqap_hook = NULL;
 
 		kvm_arch_crypto_clear_masks(kvm);
 		vfio_ap_mdev_reset_queues(matrix_mdev);
-		kvm_put_kvm(kvm);
 		matrix_mdev->kvm = NULL;
 
 		release_update_locks_for_kvm(kvm);
+		kvm_put_kvm(kvm);
 	}
 }
 
-- 
2.53.0


^ permalink raw reply related	[flat|nested] 6+ messages in thread

* Re: [PATCH v4] s390/vfio-ap: fix stale pqap_hook pointer on error in vfio_ap_mdev_set_kvm()
  2026-08-06 14:03 [PATCH v4] s390/vfio-ap: fix stale pqap_hook pointer on error in vfio_ap_mdev_set_kvm() Anthony Krowiak
@ 2026-08-06 14:18 ` sashiko-bot
  2026-08-06 18:13   ` Anthony Krowiak
  2026-08-06 14:28 ` Matthew Rosato
  2026-08-06 14:40 ` Christian Borntraeger
  2 siblings, 1 reply; 6+ messages in thread
From: sashiko-bot @ 2026-08-06 14:18 UTC (permalink / raw)
  To: Anthony Krowiak
  Cc: linux-s390, Vasily Gorbik, Alexander Gordeev, kvm, Heiko Carstens

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

Pre-existing issues:
- [High] Host AP configuration changes trigger a NULL pointer dereference when accessing unassigned matrix mediated devices.
--

--- Patch [1]: [PATCH v4] s390/vfio-ap: fix stale pqap_hook pointer on error in vfio_ap_mdev_set_kvm() ---
commit aef5cc63216cb49a3fd8ebd1da1a8d39b4fabb7b
Author: Anthony Krowiak <akrowiak@linux.ibm.com>

    s390/vfio-ap: fix stale pqap_hook pointer on error in vfio_ap_mdev_set_kvm()

In vfio_ap_mdev_set_kvm(), kvm->arch.crypto.pqap_hook is set to
&matrix_mdev->pqap_hook before the update locks are acquired and the
mdev list is checked for a conflicting assignment. If another mdev is
already attached to the same KVM instance, the function returns -EPERM
without restoring the hook pointer. This patch rearranges lock acquisition
to prevent this stale pointer issue and potential deadlocks.

> diff --git a/drivers/s390/crypto/vfio_ap_ops.c b/drivers/s390/crypto/vfio_ap_ops.c
> index 44b3a1dcc1b36..99a0efd999ef9 100644
> --- a/drivers/s390/crypto/vfio_ap_ops.c
> +++ b/drivers/s390/crypto/vfio_ap_ops.c

[ ... ]

> @@ -1883,18 +1881,15 @@ static void vfio_ap_mdev_unset_kvm(struct ap_matrix_mdev *matrix_mdev)
>  	struct kvm *kvm = matrix_mdev->kvm;
>  
>  	if (kvm && kvm->arch.crypto.crycbd) {
> -		down_write(&kvm->arch.crypto.pqap_hook_rwsem);
> -		kvm->arch.crypto.pqap_hook = NULL;
> -		up_write(&kvm->arch.crypto.pqap_hook_rwsem);
> -
>  		get_update_locks_for_kvm(kvm);
> +		kvm->arch.crypto.pqap_hook = NULL;
>  
>  		kvm_arch_crypto_clear_masks(kvm);
>  		vfio_ap_mdev_reset_queues(matrix_mdev);
> -		kvm_put_kvm(kvm);
>  		matrix_mdev->kvm = NULL;
>  
>  		release_update_locks_for_kvm(kvm);
> +		kvm_put_kvm(kvm);
>  	}
>  }

While reviewing this file, I noticed a couple of related issues.

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

[Severity: High]
This is a pre-existing issue, but if a mediated device is unassigned
(matrix_mdev->kvm is NULL) when the host AP configuration is modified,
will this unconditional lock of matrix_mdev->kvm->lock trigger a NULL pointer
dereference?


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

[Severity: High]
This isn't a bug introduced by this patch, but similar to the above, does this
function also dereference matrix_mdev->kvm without checking if it's NULL,
potentially leading to a crash when unassigned matrix mediated devices are
accessed during host AP configuration changes?

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

^ permalink raw reply	[flat|nested] 6+ messages in thread

* Re: [PATCH v4] s390/vfio-ap: fix stale pqap_hook pointer on error in vfio_ap_mdev_set_kvm()
  2026-08-06 14:03 [PATCH v4] s390/vfio-ap: fix stale pqap_hook pointer on error in vfio_ap_mdev_set_kvm() Anthony Krowiak
  2026-08-06 14:18 ` sashiko-bot
@ 2026-08-06 14:28 ` Matthew Rosato
  2026-08-06 14:40 ` Christian Borntraeger
  2 siblings, 0 replies; 6+ messages in thread
From: Matthew Rosato @ 2026-08-06 14:28 UTC (permalink / raw)
  To: Anthony Krowiak, linux-s390, linux-kernel, kvm
  Cc: jjherne, borntraeger, pasic, alex, kwankhede, hca, gor, agordeev,
	stable


> 
> Fixes: 86956e70761b3 ("s390/vfio-ap: replace open coded locks for VFIO_GROUP_NOTIFY_SET_KVM notification")
> Cc: stable@vger.kernel.org
> Signed-off-by: Anthony Krowiak <akrowiak@linux.ibm.com>
> Signed-off-by: Matthew Rosato <mjrosato@linux.ibm.com>

I think you need to re-arrange the SoB tags based on :

https://docs.kernel.org/process/submitting-patches.html#when-to-use-acked-by-cc-and-co-developed-by

Co-developed-by: Matthew Rosato <mjrosato@linux.ibm.com>
Signed-off-by: Matthew Rosato <mjrosato@linux.ibm.com>
Signed-off-by: Anthony Krowiak <akrowiak@linux.ibm.com>


^ permalink raw reply	[flat|nested] 6+ messages in thread

* Re: [PATCH v4] s390/vfio-ap: fix stale pqap_hook pointer on error in vfio_ap_mdev_set_kvm()
  2026-08-06 14:03 [PATCH v4] s390/vfio-ap: fix stale pqap_hook pointer on error in vfio_ap_mdev_set_kvm() Anthony Krowiak
  2026-08-06 14:18 ` sashiko-bot
  2026-08-06 14:28 ` Matthew Rosato
@ 2026-08-06 14:40 ` Christian Borntraeger
  2 siblings, 0 replies; 6+ messages in thread
From: Christian Borntraeger @ 2026-08-06 14:40 UTC (permalink / raw)
  To: Anthony Krowiak, linux-s390, linux-kernel, kvm
  Cc: jjherne, mjrosato, pasic, alex, kwankhede, hca, gor, agordeev,
	stable



Am 06.08.26 um 16:03 schrieb Anthony Krowiak:
> In vfio_ap_mdev_set_kvm(), kvm->arch.crypto.pqap_hook is set to
> &matrix_mdev->pqap_hook before the update locks are acquired and the
> mdev list is checked for a conflicting assignment. If another mdev is
> already attached to the same KVM instance, the function returns -EPERM
> without restoring the hook pointer, leaving kvm->arch.crypto.pqap_hook
> pointing at the failing matrix_mdev instead of the mdev that legitimately
> owns the KVM.
> 
> Since matrix_mdev->kvm is never set on this error path,
> vfio_ap_mdev_unset_kvm() will not clean up the hook when matrix_mdev
> is later closed. If matrix_mdev is subsequently freed, any PQAP
> instruction executed by the guest will dereference the stale pointer
> through pqap_hook_rwsem, resulting in a use-after-free.
> 
> Since kvm->arch.crypto.pqap_hook is only set in the vfio_ap_mdev_set_kvm()
> function and is cleared in the vfio_ap_mdev_unset_kvm() function, a check
> for 'kvm->arch.crypto.pqap_hook != NULL' is all that is needed to determine
> whether it belongs to another mdev. This will alleviate the need to iterate
> the matrix_dev->mdev_list list to see if the kvm object is assigned to
> another mdev.This was introduced in v3 to alleviate the need to take the
> mdevs_lock while iterating the list; however, this did not prevent a
> potential race condition.
> 
> The pqap_hook_rwsem(write) is now performed inside
> get_update_locks_for_kvm(), which is updated to acquire
> pqap_hook_rwsem(write) between kvm->lock and mdevs_lock. This ordering
> is consistent with the PQAP intercept path, which acquires pqap_hook_rwsem
> in read mode while srcu is held under vcpu->mutex, establishing the
> dependency: kvm->lock -> vcpu->mutex -> srcu -> pqap_hook_rwsem(read).
> 
> The pqap_hook_rwsem is now released inside the
> release_update_locks_for_kvm(), which is updated to release
> pqap_hook_rwsem(write) between mdevs_lock and kvm->lock.
> 
> Additionally, kvm_put_kvm() in vfio_ap_mdev_unset_kvm() is moved
> after release_update_locks_for_kvm(). Previously it was called while
> kvm->lock was held; if it were ever the last reference, kvm_destroy_vm()
> would run under kvm->lock, which would deadlock.
> 
> Fixes: 86956e70761b3 ("s390/vfio-ap: replace open coded locks for VFIO_GROUP_NOTIFY_SET_KVM notification")
> Cc: stable@vger.kernel.org
> Signed-off-by: Anthony Krowiak <akrowiak@linux.ibm.com>
> Signed-off-by: Matthew Rosato <mjrosato@linux.ibm.com>

Acked-by: Christian Borntraeger <borntraeger@linux.ibm.com>



^ permalink raw reply	[flat|nested] 6+ messages in thread

* Re: [PATCH v4] s390/vfio-ap: fix stale pqap_hook pointer on error in vfio_ap_mdev_set_kvm()
  2026-08-06 14:18 ` sashiko-bot
@ 2026-08-06 18:13   ` Anthony Krowiak
  2026-08-06 19:18     ` Anthony Krowiak
  0 siblings, 1 reply; 6+ messages in thread
From: Anthony Krowiak @ 2026-08-06 18:13 UTC (permalink / raw)
  To: sashiko-reviews
  Cc: linux-s390, Vasily Gorbik, Alexander Gordeev, kvm, Heiko Carstens



On 8/6/26 10: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] Host AP configuration changes trigger a NULL pointer dereference when accessing unassigned matrix mediated devices.
> --
>
> --- Patch [1]: [PATCH v4] s390/vfio-ap: fix stale pqap_hook pointer on error in vfio_ap_mdev_set_kvm() ---
> commit aef5cc63216cb49a3fd8ebd1da1a8d39b4fabb7b
> Author: Anthony Krowiak <akrowiak@linux.ibm.com>
>
>      s390/vfio-ap: fix stale pqap_hook pointer on error in vfio_ap_mdev_set_kvm()
>
> In vfio_ap_mdev_set_kvm(), kvm->arch.crypto.pqap_hook is set to
> &matrix_mdev->pqap_hook before the update locks are acquired and the
> mdev list is checked for a conflicting assignment. If another mdev is
> already attached to the same KVM instance, the function returns -EPERM
> without restoring the hook pointer. This patch rearranges lock acquisition
> to prevent this stale pointer issue and potential deadlocks.
>
>> diff --git a/drivers/s390/crypto/vfio_ap_ops.c b/drivers/s390/crypto/vfio_ap_ops.c
>> index 44b3a1dcc1b36..99a0efd999ef9 100644
>> --- a/drivers/s390/crypto/vfio_ap_ops.c
>> +++ b/drivers/s390/crypto/vfio_ap_ops.c
> [ ... ]
>
>> @@ -1883,18 +1881,15 @@ static void vfio_ap_mdev_unset_kvm(struct ap_matrix_mdev *matrix_mdev)
>>   	struct kvm *kvm = matrix_mdev->kvm;
>>   
>>   	if (kvm && kvm->arch.crypto.crycbd) {
>> -		down_write(&kvm->arch.crypto.pqap_hook_rwsem);
>> -		kvm->arch.crypto.pqap_hook = NULL;
>> -		up_write(&kvm->arch.crypto.pqap_hook_rwsem);
>> -
>>   		get_update_locks_for_kvm(kvm);
>> +		kvm->arch.crypto.pqap_hook = NULL;
>>   
>>   		kvm_arch_crypto_clear_masks(kvm);
>>   		vfio_ap_mdev_reset_queues(matrix_mdev);
>> -		kvm_put_kvm(kvm);
>>   		matrix_mdev->kvm = NULL;
>>   
>>   		release_update_locks_for_kvm(kvm);
>> +		kvm_put_kvm(kvm);
>>   	}
>>   }
> While reviewing this file, I noticed a couple of related issues.
>
> drivers/s390/crypto/vfio_ap_ops.c:vfio_ap_mdev_cfg_remove() {
> 	...
> 	list_for_each_entry(matrix_mdev, &matrix_dev->mdev_list, node) {
> 		mutex_lock(&matrix_mdev->kvm->lock);
> 		mutex_lock(&matrix_dev->mdevs_lock);
> 	...
> }
>
> [Severity: High]
> This is a pre-existing issue, but if a mediated device is unassigned
> (matrix_mdev->kvm is NULL) when the host AP configuration is modified,
> will this unconditional lock of matrix_mdev->kvm->lock trigger a NULL pointer
> dereference?
>
>
> drivers/s390/crypto/vfio_ap_ops.c:vfio_ap_mdev_hot_plug_cfg() {
> 	...
> 	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);
> 	...
> }
>
> [Severity: High]
> This isn't a bug introduced by this patch, but similar to the above, does this
> function also dereference matrix_mdev->kvm without checking if it's NULL,
> potentially leading to a crash when unassigned matrix mediated devices are
> accessed during host AP configuration changes?

Both of the above the concerns above are legitimate for the case whereby
the matrix_mdev->kvm was never set via the vfio_ap_mdev_set_kvm()
function which is called only when a guest is started with the mdev attached
and the mdev fd is opened. If the mdev is not attached to a guest, then
matrix_mdev->kvm will be NULL.

The matrix_mdev->kvm pointer is set to NULL
in the vfio_ap_mdev_unset_kvm; however, all three functions (the unset_kvm
and the two above) all take the matrix_dev->guests_lock before proceeding,
so the matrix_dev->kvm pointer is guaranteed to not be NULL due to a
race condition.

Having said that, it is clear that a check must be introduced to verify that
matrix_mdev->kvm is not NULL before taking the matrix_mdev->kvm->lock
in the two functions above.

>


^ permalink raw reply	[flat|nested] 6+ messages in thread

* Re: [PATCH v4] s390/vfio-ap: fix stale pqap_hook pointer on error in vfio_ap_mdev_set_kvm()
  2026-08-06 18:13   ` Anthony Krowiak
@ 2026-08-06 19:18     ` Anthony Krowiak
  0 siblings, 0 replies; 6+ messages in thread
From: Anthony Krowiak @ 2026-08-06 19:18 UTC (permalink / raw)
  To: sashiko-reviews
  Cc: linux-s390, Vasily Gorbik, Alexander Gordeev, kvm, Heiko Carstens,
	Jason Herne, Christian Borntraeger, Matthew Rosato,
	Claudio Imbrenda, Janosch Frank



On 8/6/26 2:13 PM, Anthony Krowiak wrote:
>
>
> On 8/6/26 10: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] Host AP configuration changes trigger a NULL pointer 
>> dereference when accessing unassigned matrix mediated devices.
>> -- 
>>
>> --- Patch [1]: [PATCH v4] s390/vfio-ap: fix stale pqap_hook pointer 
>> on error in vfio_ap_mdev_set_kvm() ---
>> commit aef5cc63216cb49a3fd8ebd1da1a8d39b4fabb7b
>> Author: Anthony Krowiak <akrowiak@linux.ibm.com>
>>
>>      s390/vfio-ap: fix stale pqap_hook pointer on error in 
>> vfio_ap_mdev_set_kvm()
>>
>> In vfio_ap_mdev_set_kvm(), kvm->arch.crypto.pqap_hook is set to
>> &matrix_mdev->pqap_hook before the update locks are acquired and the
>> mdev list is checked for a conflicting assignment. If another mdev is
>> already attached to the same KVM instance, the function returns -EPERM
>> without restoring the hook pointer. This patch rearranges lock 
>> acquisition
>> to prevent this stale pointer issue and potential deadlocks.
>>
>>> diff --git a/drivers/s390/crypto/vfio_ap_ops.c 
>>> b/drivers/s390/crypto/vfio_ap_ops.c
>>> index 44b3a1dcc1b36..99a0efd999ef9 100644
>>> --- a/drivers/s390/crypto/vfio_ap_ops.c
>>> +++ b/drivers/s390/crypto/vfio_ap_ops.c
>> [ ... ]
>>
>>> @@ -1883,18 +1881,15 @@ static void vfio_ap_mdev_unset_kvm(struct 
>>> ap_matrix_mdev *matrix_mdev)
>>>       struct kvm *kvm = matrix_mdev->kvm;
>>>         if (kvm && kvm->arch.crypto.crycbd) {
>>> -        down_write(&kvm->arch.crypto.pqap_hook_rwsem);
>>> -        kvm->arch.crypto.pqap_hook = NULL;
>>> -        up_write(&kvm->arch.crypto.pqap_hook_rwsem);
>>> -
>>>           get_update_locks_for_kvm(kvm);
>>> +        kvm->arch.crypto.pqap_hook = NULL;
>>>             kvm_arch_crypto_clear_masks(kvm);
>>>           vfio_ap_mdev_reset_queues(matrix_mdev);
>>> -        kvm_put_kvm(kvm);
>>>           matrix_mdev->kvm = NULL;
>>>             release_update_locks_for_kvm(kvm);
>>> +        kvm_put_kvm(kvm);
>>>       }
>>>   }
>> While reviewing this file, I noticed a couple of related issues.
>>
>> drivers/s390/crypto/vfio_ap_ops.c:vfio_ap_mdev_cfg_remove() {
>>     ...
>>     list_for_each_entry(matrix_mdev, &matrix_dev->mdev_list, node) {
>>         mutex_lock(&matrix_mdev->kvm->lock);
>>         mutex_lock(&matrix_dev->mdevs_lock);
>>     ...
>> }
>>
>> [Severity: High]
>> This is a pre-existing issue, but if a mediated device is unassigned
>> (matrix_mdev->kvm is NULL) when the host AP configuration is modified,
>> will this unconditional lock of matrix_mdev->kvm->lock trigger a NULL 
>> pointer
>> dereference?
>>
>>
>> drivers/s390/crypto/vfio_ap_ops.c:vfio_ap_mdev_hot_plug_cfg() {
>>     ...
>>     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);
>>     ...
>> }
>>
>> [Severity: High]
>> This isn't a bug introduced by this patch, but similar to the above, 
>> does this
>> function also dereference matrix_mdev->kvm without checking if it's 
>> NULL,
>> potentially leading to a crash when unassigned matrix mediated 
>> devices are
>> accessed during host AP configuration changes?
>
> Both of the above the concerns above are legitimate for the case whereby
> the matrix_mdev->kvm was never set via the vfio_ap_mdev_set_kvm()
> function which is called only when a guest is started with the mdev 
> attached
> and the mdev fd is opened. If the mdev is not attached to a guest, then
> matrix_mdev->kvm will be NULL.
>
> The matrix_mdev->kvm pointer is set to NULL
> in the vfio_ap_mdev_unset_kvm; however, all three functions (the 
> unset_kvm
> and the two above) all take the matrix_dev->guests_lock before 
> proceeding,
> so the matrix_dev->kvm pointer is guaranteed to not be NULL due to a
> race condition.
>
> Having said that, it is clear that a check must be introduced to 
> verify that
> matrix_mdev->kvm is not NULL before taking the matrix_mdev->kvm->lock
> in the two functions above.

The fix for this bug will be posted via a separate patch.

>
>>
>
>


^ permalink raw reply	[flat|nested] 6+ messages in thread

end of thread, other threads:[~2026-08-06 19:18 UTC | newest]

Thread overview: 6+ messages (download: mbox.gz follow: Atom feed
-- links below jump to the message on this page --
2026-08-06 14:03 [PATCH v4] s390/vfio-ap: fix stale pqap_hook pointer on error in vfio_ap_mdev_set_kvm() Anthony Krowiak
2026-08-06 14:18 ` sashiko-bot
2026-08-06 18:13   ` Anthony Krowiak
2026-08-06 19:18     ` Anthony Krowiak
2026-08-06 14:28 ` Matthew Rosato
2026-08-06 14:40 ` Christian Borntraeger

This is a public inbox, see mirroring instructions
for how to clone and mirror all data and code used for this inbox