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

[Anthony Krowiak <akrowiak@linux.ibm.com>]

On 8/6/26 1:56 PM, sashiko-bot@kernel.org wrote:
> 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 <akrowiak@linux.ibm.com>
> 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.

This was responded to for the same review comment in v4 of this patch.
A separate patch is forthcoming with a fix for this.

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