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

On 8/6/26 10:18 AM, [email protected] wrote:
> 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?

Both of the above the concerns above are legitimate for the case whereby
the matrix_mdev->kvm was never set via the vfio_ap_mdev_set_kvm()
function which is called only when a guest is started with the mdev attached
and the mdev fd is opened. If the mdev is not attached to a guest, then
matrix_mdev->kvm will be NULL.

The matrix_mdev->kvm pointer is set to NULL
in the vfio_ap_mdev_unset_kvm; however, all three functions (the unset_kvm
and the two above) all take the matrix_dev->guests_lock before proceeding,
so the matrix_dev->kvm pointer is guaranteed to not be NULL due to a
race condition.

Having said that, it is clear that a check must be introduced to verify that
matrix_mdev->kvm is not NULL before taking the matrix_mdev->kvm->lock
in the two functions above.

>