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

On 7/22/26 10:23 AM, [email protected] 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 <[email protected]>
>
> 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: [email protected]
>
>> 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;
>>   			}
>>   		}