Re: [PATCH 2/5] s390/vfio-ap: Fix dereference matrix_mdev->kvm without checking for NULL

On 8/8/26 1:36 PM, [email protected] wrote:
> Thank you for your contribution! Sashiko AI review found 3 potential issue(s) to consider:
>
> Pre-existing issues:
> - [High] Uninitialized Memory Usage in `vfio_ap_mdev_hot_plug_cfg` leads to random hardware queue resets.
> - [High] Use-After-Free in KVM's `pqap_hook` due to improper error rollback in `vfio_ap_mdev_set_kvm`.
> - [High] Unsafe concurrent modification and iteration of `matrix_dev->mdev_list` leading to Data Race / UAF.
> --
>
> --- Patch [2]: [PATCH 2/5] s390/vfio-ap: Fix dereference matrix_mdev->kvm without checking for NULL ---
> commit 9e16afa935b6610f6431f2a073e3d436ec0b7f7d
> Author: Anthony Krowiak <[email protected]>
>
> s390/vfio-ap: Fix dereference matrix_mdev->kvm without checking for NULL
>
> This commit resolves a potential NULL pointer dereference in the AP bus scan
> callbacks by explicitly checking if matrix_mdev->kvm is set before acquiring
> its lock. It also adjusts the lock acquisition order in these callbacks.
>
> Cc: [email protected]
>
>> diff --git a/drivers/s390/crypto/vfio_ap_ops.c b/drivers/s390/crypto/vfio_ap_ops.c
>> index 845c86ba8bc30..e382e5a1cb996 100644
>> --- a/drivers/s390/crypto/vfio_ap_ops.c
>> +++ b/drivers/s390/crypto/vfio_ap_ops.c
> [ ... ]
>
>> @@ -2821,9 +2828,6 @@ static void vfio_ap_mdev_hot_plug_cfg(struct ap_matrix_mdev *matrix_mdev)
>>   	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);
>> -
>>   	filter_adapters = bitmap_intersects(matrix_mdev->matrix.apm,
>>   					    matrix_mdev->apm_add, AP_DEVICES);
>>   	filter_domains = bitmap_intersects(matrix_mdev->matrix.aqm,
>> @@ -2841,9 +2845,6 @@ static void vfio_ap_mdev_hot_plug_cfg(struct ap_matrix_mdev *matrix_mdev)
>>   		vfio_ap_mdev_update_guest_apcb(matrix_mdev);
>>   
>>   	reset_queues_for_apids(matrix_mdev, apm_filtered);
>> https://lkml.org/lkml/2026/8/6/1775
> [Severity: High]
> This is a pre-existing issue, but does this function pass an uninitialized
> bitmap to reset_queues_for_apids()?
>
> If the AP configuration only adds control domains, filter_cdoms is true, but
> both filter_adapters and filter_domains are false. This skips the call to
> vfio_ap_mdev_filter_matrix() which initializes apm_filtered:
>
>      if (filter_adapters || filter_domains)
>          do_hotplug = vfio_ap_mdev_filter_matrix(matrix_mdev, apm_filtered);
>
> If this initialization path is bypassed, is it possible for apm_filtered to
> retain stack garbage when it is subsequently passed to
> reset_queues_for_apids(), potentially causing unintended guest hardware
> queues to be reset?

Yes, because if no adapters or domains need to be filtered, the call to
vfio_ap_mdev_filter_matrix - which zeros all bits to apm_filtered - will 
be bypassed.
The apm_filtered bitmap needs to be zeroed before the check for 
filter_adapters
and filter_domains.

>
>> -
>> -	mutex_unlock(&matrix_dev->mdevs_lock);
>> -	mutex_unlock(&matrix_mdev->kvm->lock);
>>   }
>>   
>>   void vfio_ap_on_scan_complete(struct ap_config_info *new_config_info,
>> @@ -2854,15 +2855,29 @@ void vfio_ap_on_scan_complete(struct ap_config_info *new_config_info,
>>   	mutex_lock(&matrix_dev->guests_lock);
>>   
>>   	list_for_each_entry(matrix_mdev, &matrix_dev->mdev_list, node) {
> [Severity: High]
> This is a pre-existing issue, but could this list iteration race with
> concurrent list modifications?
>
> It appears this list traversal in vfio_ap_on_scan_complete() occurs while
> holding only matrix_dev->guests_lock at the list level. However, list
> mutations such as the one in vfio_ap_mdev_probe():
>
> vfio_ap_mdev_probe() {
>      ...
>      mutex_lock(&matrix_dev->mdevs_lock);
>      list_add(&matrix_mdev->node, &matrix_dev->mdev_list);
>      mutex_unlock(&matrix_dev->mdevs_lock);
>      ...
> }
>
> are protected only by matrix_dev->mdevs_lock. Since the internal iteration
> step happens outside of matrix_dev->mdevs_lock, can this expose the reader to
> torn or stale pointers, potentially leading to a use-after-free or infinite
> loop if an administrator concurrently creates a new mdev?

This is fixed via patch 3/5 of the series in which this patch is included.

>
>> +		/*
>> +		 * The mdevs_lock must be held to access fields within matrix_mdev,
>> +		 * and kvm->lock must be taken before mdevs_lock to satisfy the lock
>> +		 * ordering requirement and prevent a lockdep splat.
>> +		 */
> [ ... ]
>
> While reviewing this file, I also noticed another potential issue outside the
> immediate diff in vfio_ap_mdev_set_kvm():
>
>>        kvm->arch.crypto.pqap_hook = &matrix_mdev->pqap_hook;
>>        ...
>>        if (m != matrix_mdev && m->kvm == kvm) {
>>                release_update_locks_for_kvm(kvm);
>>                return -EPERM;
>>        }
> [Severity: High]
> This is a pre-existing issue, but is it possible for KVM's pqap_hook to be
> left pointing to freed memory upon an error?
>
> The KVM hook is unconditionally updated to point to the incoming matrix_mdev.
> However, if another mdev is already attached to this KVM instance, the
> function bails out with -EPERM.
>
> If the rejected mdev is subsequently freed by userspace, would KVM retain a
> dangling pointer? Could this cause a use-after-free when a guest later
> issues a PQAP instruction and handle_pqap() dereferences the hook?

A fix for this has already been submitted to the kernel mailing list:
https://lkml.org/lkml/2026/8/6/1775

>