Re: [PATCH v5] 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 1 potential issue(s) to consider:

Pre-existing issues:
- [High] Unconditional dereference of `matrix_mdev->kvm` when handling host AP configuration changes.
--

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

This commit fixes a stale pointer use-after-free issue when another mdev is 
already attached to a KVM instance. It avoids iterating the mdev_list under 
the mdevs_lock by changing the rwsem lock order and checking if 
kvm->arch.crypto.pqap_hook is already set.

> diff --git a/drivers/s390/crypto/vfio_ap_ops.c b/drivers/s390/crypto/vfio_ap_ops.c
> index 44b3a1dcc1b36..99a0efd999ef9 100644
> --- a/drivers/s390/crypto/vfio_ap_ops.c
> +++ b/drivers/s390/crypto/vfio_ap_ops.c
[ ... ]
> @@ -1883,18 +1881,15 @@ static void vfio_ap_mdev_unset_kvm(struct ap_matrix_mdev *matrix_mdev)
>  	struct kvm *kvm = matrix_mdev->kvm;
>  
>  	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);
> +		kvm->arch.crypto.pqap_hook = NULL;
>  
>  		kvm_arch_crypto_clear_masks(kvm);
>  		vfio_ap_mdev_reset_queues(matrix_mdev);
> -		kvm_put_kvm(kvm);
>  		matrix_mdev->kvm = NULL;

[Severity: High]
This is a pre-existing issue, but can unattached mdevs cause a crash during
host AP configuration changes?

If an mdev is created but not attached to a guest (meaning matrix_mdev->kvm
is NULL), and a host AP config change occurs, vfio_ap_on_cfg_changed()
eventually triggers vfio_ap_mdev_cfg_remove():

drivers/s390/crypto/vfio_ap_ops.c:vfio_ap_mdev_cfg_remove() {
    ...
	list_for_each_entry(matrix_mdev, &matrix_dev->mdev_list, node) {
		mutex_lock(&matrix_mdev->kvm->lock);
    ...
}

Will this unconditionally dereference matrix_mdev->kvm and cause a panic
since it isn't checked for NULL?  A similar path exists in
vfio_ap_mdev_hot_plug_cfg() as well.

>  
>  		release_update_locks_for_kvm(kvm);
> +		kvm_put_kvm(kvm);
>  	}
>  }

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