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

On 7/28/26 12:41 PM, [email protected] wrote:
> Thank you for your contribution! Sashiko AI review found 1 potential issue(s) to consider:
> - [High] Removing the `mdev_list` iteration from within the `update_locks` critical section introduces a Time-Of-Check to Time-Of-Use (TOCTOU) race condition between `vfio_ap_mdev_set_kvm()` and `vfio_ap_mdev_unset_kvm()` that silently clobbers KVM APCB masks for a newly attached mdev.
> --
>
> --- Patch [1]: [PATCH v2] s390/vfio-ap: fix stale pqap_hook pointer on error in vfio_ap_mdev_set_kvm() ---
> commit 0834fa20c3fb715cac9e1ff09376e43784d44814
> Author: Anthony Krowiak <[email protected]>
> s390/vfio-ap: fix stale pqap_hook pointer on error in vfio_ap_mdev_set_kvm()
>
> This commit changes vfio_ap_mdev_set_kvm() to use pqap_hook to detect
> conflicting assignments instead of iterating mdev_list under the update locks,
> resolving a potential stale pointer dereference.
>
> Fixes: 86956e70761b3 ("s390/vfio-ap: replace open coded locks for VFIO_GROUP_NOTIFY_SET_KVM notification")
> Cc: [email protected]
>
>> diff --git a/drivers/s390/crypto/vfio_ap_ops.c b/drivers/s390/crypto/vfio_ap_ops.c
>> index 44b3a1dcc1b36..b0a038faa93ac 100644
>> --- a/drivers/s390/crypto/vfio_ap_ops.c
>> +++ b/drivers/s390/crypto/vfio_ap_ops.c
> [ ... ]
>> @@ -1821,26 +1821,25 @@ 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) {
>> +		/*
>> +		 * 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
>> +		 * check and update the pqap_hook.
>> +		 */
>>   		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]
> Can this introduce a race with vfio_ap_mdev_unset_kvm()?
>
> By moving the conflict check outside of the update_locks critical section,
> is it possible for the following sequence to occur during concurrent
> hot-plug/unplug operations?
>
> Thread 1 (in vfio_ap_mdev_unset_kvm):
>      down_write(&kvm->arch.crypto.pqap_hook_rwsem);
>      kvm->arch.crypto.pqap_hook = NULL;
>      up_write(&kvm->arch.crypto.pqap_hook_rwsem);
>
> Thread 2 (in vfio_ap_mdev_set_kvm):
>      down_write(&kvm->arch.crypto.pqap_hook_rwsem);
>      // Sees pqap_hook is NULL
>      kvm->arch.crypto.pqap_hook = &matrix_mdev->pqap_hook;
>      up_write(&kvm->arch.crypto.pqap_hook_rwsem);
>      get_update_locks_for_kvm(kvm);
>      vfio_ap_mdev_update_guest_apcb(matrix_mdev);
>      release_update_locks_for_kvm(kvm);
>
> Thread 1 (continuing in vfio_ap_mdev_unset_kvm):
>      get_update_locks_for_kvm(kvm);
>      kvm_arch_crypto_clear_masks(kvm);
>
> If Thread 1 interleaves after Thread 2 sets the guest masks but before Thread
> 1 clears them, would this result in Thread 1 unconditionally clearing the KVM
> APCB masks that Thread 2 just successfully configured, leaving the newly
> attached mdev with inaccessible queues?

This scenario requires Thread 1 (unset_kvm) to be tearing down the same
matrix_mdev that Thread 2 (set_kvm) is attaching. But look at the entry
conditions:

* vfio_ap_mdev_unset_kvm is called from vfio_ap_mdev_close_device
* vfio_ap_mdev_set_kvm is called from vfio_ap_mdev_open_device

open_device and close_device on the same vfio_device are serialized by the
vfio core under dev_set->lock; you cannot have both executing concurrently
for the same mdev.

Furthermore, vfio_ap_mdev_unset_kvm reads matrix_mdev->kvm which is only
set to a non-NULL value inside vfio_ap_mdev_set_kvm. For Thread 1's
unset_kvm to operate on the same kvm as Thread 2's set_kvm, Thread 1's
matrix_mdev would have to have already gone through a complete set_kvm
cycle and have ->kvm set; in other words, it is a different matrix_mdev
that previously attached to the same KVM.

This fix prevents that: the pqap_hook != NULL check blocks Thread 2 from
attaching to a KVM that already has an owner. So Thread 1 and Thread 2 can
never legitimately be racing over the same kvm in the scenario described.

>
>> -
>> -		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);
>>   	}
>>