Re: [PATCH v4] 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 2:13 PM, Anthony Krowiak wrote:
>
>
> On 8/6/26 10:18 AM, sashiko-bot@kernel.org 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 <akrowiak@linux.ibm.com>
>>
>>      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.

The fix for this bug will be posted via a separate patch.

>
>>
>
>