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

Thank you for your contribution! Sashiko AI review found 2 potential issue(s) to consider:

Pre-existing issues:
- [High] The `apm_filtered` stack array is conditionally left uninitialized but unconditionally used, causing random AP queue resets.
- [High] Concurrent list additions and traversals cause unprotected data races on `matrix_dev->mdev_list`.
--

--- Patch [2]: [PATCH v5 2/9] s390/vfio-ap: Fix dereference matrix_mdev->kvm without checking for NULL ---
commit da7c08a88a7102b1daed33f3e6352fe4dffb203f
Author: Anthony Krowiak <[email protected]>

s390/vfio-ap: Fix dereference matrix_mdev->kvm without checking for NULL

This patch adds checks to ensure matrix_mdev->kvm is not NULL before taking
the matrix_mdev->kvm->lock mutex during the configuration change and scan
complete callbacks. The locking order between kvm->lock and mdevs_lock is
also updated.

> diff --git a/drivers/s390/crypto/vfio_ap_ops.c b/drivers/s390/crypto/vfio_ap_ops.c
> index 845c86ba8bc30..c6bee69cc22f6 100644
> --- a/drivers/s390/crypto/vfio_ap_ops.c
> +++ b/drivers/s390/crypto/vfio_ap_ops.c
[ ... ]
> @@ -2821,9 +2834,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 +2851,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);

[Severity: High]
This isn't a bug introduced by this patch, but can this sequence result in
reading uninitialized stack memory?

In vfio_ap_mdev_hot_plug_cfg(), DECLARE_BITMAP() allocates apm_filtered on
the stack but does not initialize it.

If filter_adapters and filter_domains are both false (for example, if only
control domains were added), the code bypasses vfio_ap_mdev_filter_matrix(),
leaving the stack array completely uninitialized.

When reset_queues_for_apids() is then called, will it unconditionally iterate
over this garbage memory and potentially trigger unintended hardware queue
resets (ap_zapq) for random matching queues?

> -
> -	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 +2861,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 does this list traversal have an
unprotected data race?

The matrix_dev->mdev_list is traversed here holding only
matrix_dev->guests_lock. Because matrix_dev->mdevs_lock is acquired inside
the loop, it is dropped before the list_next_entry() macro step evaluates
the next pointer for the iteration.

Since vfio_ap_mdev_probe() adds devices to this list while holding only
matrix_dev->mdevs_lock:

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);
    ...
}

Could a concurrent bus scan and probe cause this traversal in
vfio_ap_on_scan_complete() to read a partially updated list pointer and
cause list corruption?

> +		/*
> +		 * 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.
> +		 */
> +		if (matrix_mdev->kvm)
> +			mutex_lock(&matrix_mdev->kvm->lock);
> +		mutex_lock(&matrix_dev->mdevs_lock);
> +
>  		if (bitmap_empty(matrix_mdev->apm_add, AP_DEVICES) &&
>  		    bitmap_empty(matrix_mdev->aqm_add, AP_DOMAINS) &&
>  		    bitmap_empty(matrix_mdev->adm_add, AP_DOMAINS))
[ ... ]

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