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

Thank you for your contribution! Sashiko AI review found 2 potential issue(s) to consider:

New issues:
- [High] Lock inversion between KVM's SRCU (`kvm->srcu`), `pqap_hook_rwsem`, and `kvm->lock` leads to circular deadlocks.

Pre-existing issues:
- [Critical] Calling `kvm_put_kvm(kvm)` before releasing `kvm->lock` and `pqap_hook_rwsem` leads to a potential Use-After-Free of the locks embedded in the `kvm` structure.
--

--- Patch [1]: [PATCH v3] s390/vfio-ap: fix stale pqap_hook pointer on error in vfio_ap_mdev_set_kvm() ---
commit 5f259db058dc54a89b0a65f6eb549bf2d44cc6f7
Author: Anthony Krowiak <[email protected]>

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

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.

Cc: [email protected]

> diff --git a/drivers/s390/crypto/vfio_ap_ops.c b/drivers/s390/crypto/vfio_ap_ops.c
> index 44b3a1dcc1b36..f9460310d47b1 100644
> --- a/drivers/s390/crypto/vfio_ap_ops.c
> +++ b/drivers/s390/crypto/vfio_ap_ops.c
> @@ -1821,27 +1821,20 @@ 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) {
>  		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]
Does acquiring the locks in this order create a circular deadlock with KVM's
SRCU and kvm->lock?

If a guest executes a PQAP instruction, the KVM intercept handler runs with
srcu_read_lock(&kvm->srcu) held and acquires pqap_hook_rwsem in read mode.

By holding pqap_hook_rwsem in write mode here across
get_update_locks_for_kvm(), which acquires kvm->lock, this introduces a lock
dependency: pqap_hook_rwsem -> kvm->lock.

If a concurrent KVM core operation acquires kvm->lock and then calls
synchronize_srcu(&kvm->srcu), this creates a 3-way circular deadlock:
kvm->srcu -> pqap_hook_rwsem -> kvm->lock -> kvm->srcu.

[ ... ]

> @@ -1885,7 +1878,6 @@ static void vfio_ap_mdev_unset_kvm(struct ap_matrix_mdev *matrix_mdev)
>  	if (kvm && kvm->arch.crypto.crycbd) {
>  		down_write(&kvm->arch.crypto.pqap_hook_rwsem);
>  		kvm->arch.crypto.pqap_hook = NULL;
> -		up_write(&kvm->arch.crypto.pqap_hook_rwsem);
>  
>  		get_update_locks_for_kvm(kvm);
>  
> @@ -1895,6 +1887,7 @@ static void vfio_ap_mdev_unset_kvm(struct ap_matrix_mdev *matrix_mdev)
>  		matrix_mdev->kvm = NULL;
>  
>  		release_update_locks_for_kvm(kvm);
> +		up_write(&kvm->arch.crypto.pqap_hook_rwsem);

[Severity: Critical]
This is a pre-existing issue, but does dropping the KVM reference before
releasing the locks lead to a use-after-free of the kvm structure?

If userspace closes the KVM file descriptor before closing the mdev file
descriptor, the initial KVM reference is dropped. When kvm_put_kvm(kvm)
is called just before this block in vfio_ap_mdev_unset_kvm(), it drops
the final reference and synchronously frees the kvm structure.

The subsequent calls to release_update_locks_for_kvm() and
up_write(&kvm->arch.crypto.pqap_hook_rwsem) will then attempt to unlock
mutexes within the freed kvm struct, which can corrupt memory.

>  	}
>  }

-- 
Sashiko AI review · https://sashiko.dev/#/patchset/[email protected]?part=1