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

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

Fix this by restoring kvm->arch.crypto.pqap_hook to &m->pqap_hook on
the -EPERM path, where m is the already-attached mdev discovered during
the conflict check. This is the value the hook held before it was
overwritten, and it is the only safe value to restore since m remains
live and its pqap_hook is the handler the guest must continue to use.

The rwsem is re-taken under write before restoring the pointer, which
is consistent with the existing locking pattern and ensures no VCPU
thread can observe a torn value.

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

diff --git a/drivers/s390/crypto/vfio_ap_ops.c b/drivers/s390/crypto/vfio_ap_ops.c
index 44b3a1dcc1b3..e18540fd6dc7 100644
--- a/drivers/s390/crypto/vfio_ap_ops.c
+++ b/drivers/s390/crypto/vfio_ap_ops.c
@@ -1824,6 +1824,12 @@ static int vfio_ap_mdev_set_kvm(struct ap_matrix_mdev *matrix_mdev,
 	struct ap_matrix_mdev *m;
 
 	if (kvm->arch.crypto.crycbd) {
+		/*
+		 * Taking the kvm->arch.crypto.pqap_hook_rwsem lock while
+		 * holding the update locks (below) could result in a
+		 * deadlock condition, so let's hold the rwsem here while we
+		 * update the pqap_hook.
+		 */
 		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);
@@ -1833,6 +1839,10 @@ static int vfio_ap_mdev_set_kvm(struct ap_matrix_mdev *matrix_mdev,
 		list_for_each_entry(m, &matrix_dev->mdev_list, node) {
 			if (m != matrix_mdev && m->kvm == kvm) {
 				release_update_locks_for_kvm(kvm);
+				/* Undo the hook installation since we're failing */
+				down_write(&kvm->arch.crypto.pqap_hook_rwsem);
+				kvm->arch.crypto.pqap_hook = &m->pqap_hook;
+				up_write(&kvm->arch.crypto.pqap_hook_rwsem);
 				return -EPERM;
 			}
 		}
-- 
2.53.0

Re: [PATCH] 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 1 potential issue(s) to consider:
- [Critical] Restoring `kvm->arch.crypto.pqap_hook` to `&m->pqap_hook` after releasing update locks creates a TOCTOU race condition leading to an exploitable Use-After-Free.
--

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

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

This commit addresses an issue where the pqap_hook pointer is left pointing
at a failing matrix_mdev when a conflicting assignment is detected. It fixes
this by restoring the hook to the legitimately attached mdev discovered
during the error path.

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..e18540fd6dc75 100644
> --- a/drivers/s390/crypto/vfio_ap_ops.c
> +++ b/drivers/s390/crypto/vfio_ap_ops.c

[ ... ]

> @@ -1833,6 +1839,10 @@ static int vfio_ap_mdev_set_kvm(struct ap_matrix_mdev *matrix_mdev,
>  		list_for_each_entry(m, &matrix_dev->mdev_list, node) {
>  			if (m != matrix_mdev && m->kvm == kvm) {
>  				release_update_locks_for_kvm(kvm);
> +				/* Undo the hook installation since we're failing */
> +				down_write(&kvm->arch.crypto.pqap_hook_rwsem);
> +				kvm->arch.crypto.pqap_hook = &m->pqap_hook;

[Severity: Critical]
Is there a race condition introduced here since the update locks are released
right before restoring the hook pointer?

Looking at vfio_ap_mdev_set_kvm() in drivers/s390/crypto/vfio_ap_ops.c, 
release_update_locks_for_kvm() drops the mutexes that protect the mdev
list. Once those locks are dropped, could another thread concurrently remove
and free the existing mdev (m)?

If m is freed while we are waiting to acquire the pqap_hook_rwsem, this code
would then assign a dangling pointer to kvm->arch.crypto.pqap_hook. Later,
when the guest executes a PQAP instruction, it might dereference this freed
memory, resulting in a use-after-free.

Would it be safer to restore the hook pointer before calling
release_update_locks_for_kvm() to ensure m remains valid?

> +				up_write(&kvm->arch.crypto.pqap_hook_rwsem);
>  				return -EPERM;
>  			}
>  		}

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

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

[Anthony Krowiak]

On 7/22/26 10:23 AM, sashiko-bot@kernel.org wrote:
> Thank you for your contribution! Sashiko AI review found 1 potential issue(s) to consider:
> - [Critical] Restoring `kvm->arch.crypto.pqap_hook` to `&m->pqap_hook` after releasing update locks creates a TOCTOU race condition leading to an exploitable Use-After-Free.
> --
>
> --- Patch [1]: [PATCH] s390/vfio-ap: fix stale pqap_hook pointer on error in vfio_ap_mdev_set_kvm() ---
> commit 3eedf7c518752f33e7b7b7758dc2098b54d797b9
> Author: Anthony Krowiak <akrowiak@linux.ibm.com>
>
> s390/vfio-ap: fix stale pqap_hook pointer on error in vfio_ap_mdev_set_kvm()
>
> This commit addresses an issue where the pqap_hook pointer is left pointing
> at a failing matrix_mdev when a conflicting assignment is detected. It fixes
> this by restoring the hook to the legitimately attached mdev discovered
> during the error path.
>
> 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..e18540fd6dc75 100644
>> --- a/drivers/s390/crypto/vfio_ap_ops.c
>> +++ b/drivers/s390/crypto/vfio_ap_ops.c
> [ ... ]
>
>> @@ -1833,6 +1839,10 @@ static int vfio_ap_mdev_set_kvm(struct ap_matrix_mdev *matrix_mdev,
>>   		list_for_each_entry(m, &matrix_dev->mdev_list, node) {
>>   			if (m != matrix_mdev && m->kvm == kvm) {
>>   				release_update_locks_for_kvm(kvm);
>> +				/* Undo the hook installation since we're failing */
>> +				down_write(&kvm->arch.crypto.pqap_hook_rwsem);
>> +				kvm->arch.crypto.pqap_hook = &m->pqap_hook;
> [Severity: Critical]
> Is there a race condition introduced here since the update locks are released
> right before restoring the hook pointer?

This is a legitimate point since it could result in a use after free 
problem accessing
m->pqap_hook while not under the mdevs_lock. This needs to be fixed.

>
> Looking at vfio_ap_mdev_set_kvm() in drivers/s390/crypto/vfio_ap_ops.c,
> release_update_locks_for_kvm() drops the mutexes that protect the mdev
> list. Once those locks are dropped, could another thread concurrently remove
> and free the existing mdev (m)?
>
> If m is freed while we are waiting to acquire the pqap_hook_rwsem, this code
> would then assign a dangling pointer to kvm->arch.crypto.pqap_hook. Later,
> when the guest executes a PQAP instruction, it might dereference this freed
> memory, resulting in a use-after-free.
>
> Would it be safer to restore the hook pointer before calling
> release_update_locks_for_kvm() to ensure m remains valid?

The pqap_hook.rw_sem semaphore can not be held while the mdevs_lock
mutex acquired via the get_update_locks_for_kvm() function are held. 
This will result in
a deadlock when the pqap_hook function is executed because it will be held
while the hook is executing and the hook callback will try to acquire the
mdevs_lock, thus resulting in a deadlock.

>
>> +				up_write(&kvm->arch.crypto.pqap_hook_rwsem);
>>   				return -EPERM;
>>   			}
>>   		}