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

[sashiko-bot@kernel.org]

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 <akrowiak@linux.ibm.com>
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: stable@vger.kernel.org

> 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?

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

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