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

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

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

Note that the 'matrix_mdev' pointer retrieved via the 'container_of' macro
in the 'vfio_ap_mdev_open_device()' callback will be available for the
duration of that function because 'matrix_mdev' cannot be freed while
'open_device' (which calls the 'vfio_ap_mdev_open_device()' function) is
running.

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

diff --git a/drivers/s390/crypto/vfio_ap_ops.c b/drivers/s390/crypto/vfio_ap_ops.c
index 44b3a1dcc1b3..f9460310d47b 100644
--- a/drivers/s390/crypto/vfio_ap_ops.c
+++ b/drivers/s390/crypto/vfio_ap_ops.c
@@ -1821,27 +1821,20 @@ 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);
+		if (kvm->arch.crypto.pqap_hook) {
+			up_write(&kvm->arch.crypto.pqap_hook_rwsem);
+			return -EPERM;
+		}
 		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;
-			}
-		}
-
 		kvm_get_kvm(kvm);
 		matrix_mdev->kvm = kvm;
 		vfio_ap_mdev_update_guest_apcb(matrix_mdev);
-
 		release_update_locks_for_kvm(kvm);
+		up_write(&kvm->arch.crypto.pqap_hook_rwsem);
 	}
 
 	return 0;
@@ -1885,7 +1878,6 @@ static void vfio_ap_mdev_unset_kvm(struct ap_matrix_mdev *matrix_mdev)
 	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);
 
@@ -1895,6 +1887,7 @@ static void vfio_ap_mdev_unset_kvm(struct ap_matrix_mdev *matrix_mdev)
 		matrix_mdev->kvm = NULL;
 
 		release_update_locks_for_kvm(kvm);
+		up_write(&kvm->arch.crypto.pqap_hook_rwsem);
 	}
 }
 
-- 
2.53.0

Re: [PATCH v3] s390/vfio-ap: fix stale pqap_hook pointer on error in vfio_ap_mdev_set_kvm()

[sashiko-bot]

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

New issues:
- [High] Lock inversion between KVM's SRCU (`kvm->srcu`), `pqap_hook_rwsem`, and `kvm->lock` leads to circular deadlocks.

Pre-existing issues:
- [Critical] Calling `kvm_put_kvm(kvm)` before releasing `kvm->lock` and `pqap_hook_rwsem` leads to a potential Use-After-Free of the locks embedded in the `kvm` structure.
--

--- Patch [1]: [PATCH v3] s390/vfio-ap: fix stale pqap_hook pointer on error in vfio_ap_mdev_set_kvm() ---
commit 5f259db058dc54a89b0a65f6eb549bf2d44cc6f7
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.

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..f9460310d47b1 100644
> --- a/drivers/s390/crypto/vfio_ap_ops.c
> +++ b/drivers/s390/crypto/vfio_ap_ops.c
> @@ -1821,27 +1821,20 @@ 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);
> +		if (kvm->arch.crypto.pqap_hook) {
> +			up_write(&kvm->arch.crypto.pqap_hook_rwsem);
> +			return -EPERM;
> +		}
>  		kvm->arch.crypto.pqap_hook = &matrix_mdev->pqap_hook;
> -		up_write(&kvm->arch.crypto.pqap_hook_rwsem);
>  
>  		get_update_locks_for_kvm(kvm);

[Severity: High]
Does acquiring the locks in this order create a circular deadlock with KVM's
SRCU and kvm->lock?

If a guest executes a PQAP instruction, the KVM intercept handler runs with
srcu_read_lock(&kvm->srcu) held and acquires pqap_hook_rwsem in read mode.

By holding pqap_hook_rwsem in write mode here across
get_update_locks_for_kvm(), which acquires kvm->lock, this introduces a lock
dependency: pqap_hook_rwsem -> kvm->lock.

If a concurrent KVM core operation acquires kvm->lock and then calls
synchronize_srcu(&kvm->srcu), this creates a 3-way circular deadlock:
kvm->srcu -> pqap_hook_rwsem -> kvm->lock -> kvm->srcu.

[ ... ]

> @@ -1885,7 +1878,6 @@ static void vfio_ap_mdev_unset_kvm(struct ap_matrix_mdev *matrix_mdev)
>  	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);
>  
> @@ -1895,6 +1887,7 @@ static void vfio_ap_mdev_unset_kvm(struct ap_matrix_mdev *matrix_mdev)
>  		matrix_mdev->kvm = NULL;
>  
>  		release_update_locks_for_kvm(kvm);
> +		up_write(&kvm->arch.crypto.pqap_hook_rwsem);

[Severity: Critical]
This is a pre-existing issue, but does dropping the KVM reference before
releasing the locks lead to a use-after-free of the kvm structure?

If userspace closes the KVM file descriptor before closing the mdev file
descriptor, the initial KVM reference is dropped. When kvm_put_kvm(kvm)
is called just before this block in vfio_ap_mdev_unset_kvm(), it drops
the final reference and synchronously frees the kvm structure.

The subsequent calls to release_update_locks_for_kvm() and
up_write(&kvm->arch.crypto.pqap_hook_rwsem) will then attempt to unlock
mutexes within the freed kvm struct, which can corrupt memory.

>  	}
>  }

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