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

[Christian Borntraeger]

Am 06.08.26 um 16:03 schrieb 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.This was introduced in v3 to alleviate the need to take the
> mdevs_lock while iterating the list; however, this did not prevent a
> potential race condition.
>
> The pqap_hook_rwsem(write) is now performed inside
> get_update_locks_for_kvm(), which is updated to acquire
> pqap_hook_rwsem(write) between kvm->lock and mdevs_lock. This ordering
> is consistent with the PQAP intercept path, which acquires pqap_hook_rwsem
> in read mode while srcu is held under vcpu->mutex, establishing the
> dependency: kvm->lock -> vcpu->mutex -> srcu -> pqap_hook_rwsem(read).
>
> The pqap_hook_rwsem is now released inside the
> release_update_locks_for_kvm(), which is updated to release
> pqap_hook_rwsem(write) between mdevs_lock and kvm->lock.
>
> Additionally, kvm_put_kvm() in vfio_ap_mdev_unset_kvm() is moved
> after release_update_locks_for_kvm(). Previously it was called while
> kvm->lock was held; if it were ever the last reference, kvm_destroy_vm()
> would run under kvm->lock, which would deadlock.
>
> Fixes: 86956e70761b3 ("s390/vfio-ap: replace open coded locks for VFIO_GROUP_NOTIFY_SET_KVM notification")
> Cc: stable@xxxxxxxxxxxxxxx
> Signed-off-by: Anthony Krowiak <akrowiak@xxxxxxxxxxxxx>
> Signed-off-by: Matthew Rosato <mjrosato@xxxxxxxxxxxxx>

Acked-by: Christian Borntraeger <borntraeger@xxxxxxxxxxxxx>