Re: [PATCH v4] 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] Host AP configuration changes trigger a NULL pointer dereference when accessing unassigned matrix mediated devices.
--

--- Patch [1]: [PATCH v4] s390/vfio-ap: fix stale pqap_hook pointer on error in vfio_ap_mdev_set_kvm() ---
commit aef5cc63216cb49a3fd8ebd1da1a8d39b4fabb7b
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. If another mdev is
already attached to the same KVM instance, the function returns -EPERM
without restoring the hook pointer. This patch rearranges lock acquisition
to prevent this stale pointer issue and potential deadlocks.

> 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;
>  
>  		release_update_locks_for_kvm(kvm);
> +		kvm_put_kvm(kvm);
>  	}
>  }

While reviewing this file, I noticed a couple of related issues.

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);
		mutex_lock(&matrix_dev->mdevs_lock);
	...
}

[Severity: High]
This is a pre-existing issue, but if a mediated device is unassigned
(matrix_mdev->kvm is NULL) when the host AP configuration is modified,
will this unconditional lock of matrix_mdev->kvm->lock trigger a NULL pointer
dereference?


drivers/s390/crypto/vfio_ap_ops.c:vfio_ap_mdev_hot_plug_cfg() {
	...
	DECLARE_BITMAP(apm_filtered, AP_DEVICES);
	bool filter_domains, filter_adapters, filter_cdoms, do_hotplug = false;

	mutex_lock(&matrix_mdev->kvm->lock);
	mutex_lock(&matrix_dev->mdevs_lock);
	...
}

[Severity: High]
This isn't a bug introduced by this patch, but similar to the above, does this
function also dereference matrix_mdev->kvm without checking if it's NULL,
potentially leading to a crash when unassigned matrix mediated devices are
accessed during host AP configuration changes?

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