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

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

Pre-existing issues:
- [High] Uninitialized stack-allocated bitmap (`apm_filtered`) is passed to `reset_queues_for_apids`.
- [High] Missing matrix_dev->mdevs_lock acquisition in vfio_ap_mdev_cfg_add() causing data race on matrix_mdev fields.
- [High] Unsafe iteration over matrix_dev->mdev_list without holding mdevs_lock.
--

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

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

This commit adds NULL checks for matrix_mdev->kvm before attempting to
acquire the matrix_mdev->kvm->lock mutex in the configuration change and
scan complete callbacks. This prevents a potential NULL pointer dereference
if the kvm pointer has not been initialized.

> --- a/drivers/s390/crypto/vfio_ap_ops.c
> +++ b/drivers/s390/crypto/vfio_ap_ops.c
> @@ -2606,7 +2606,13 @@ static void vfio_ap_mdev_cfg_remove(unsigned long *ap_remove,
>  	int do_remove;
>  
>  	list_for_each_entry(matrix_mdev, &matrix_dev->mdev_list, node) {

[Severity: High]
This isn't a bug introduced by this patch, but is it safe to iterate over
mdev_list holding only matrix_dev->guests_lock here?

Looking at vfio_ap_mdev_probe(), it appears new entries are added to
this list holding only matrix_dev->mdevs_lock (without taking guests_lock).

Could concurrent probing during an AP bus scan modify the list while we are
traversing it locklessly here, leading to list corruption?

> -		mutex_lock(&matrix_mdev->kvm->lock);
> +		/*
> +		 * 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);

[Severity: High]
This is a pre-existing issue, but does vfio_ap_mdev_cfg_add() also need
matrix_dev->mdevs_lock protection like this?

vfio_ap_mdev_cfg_add() is called right after vfio_ap_mdev_cfg_remove() from
vfio_ap_on_cfg_changed(), which only holds matrix_dev->guests_lock. It
modifies matrix_mdev->apm_add, aqm_add, and adm_add, and reads
matrix_mdev->matrix.

Could concurrent modifications race without mdevs_lock being held and
corrupt the guest AP configuration bitmaps in vfio_ap_mdev_cfg_add()?

[ ... ]
> @@ -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;

[Severity: High]
This is a pre-existing issue, but does apm_filtered need to be zeroed or
initialized before use?

If vfio_ap_on_scan_complete() is called with non-empty adm_add but empty
apm_add and aqm_add, filter_adapters and filter_domains are set to false.
This bypasses vfio_ap_mdev_filter_matrix(), which normally initializes
apm_filtered.

Then the uninitialized stack variable is passed down:

    reset_queues_for_apids(matrix_mdev, apm_filtered);

Could this cause reset_queues_for_apids() to iterate over random stack
garbage bits and inadvertently reset arbitrary AP queues?

[ ... ]
>  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 isn't a bug introduced by this patch, but similar to the loop in
vfio_ap_mdev_cfg_remove() above, does this iteration also need
matrix_dev->mdevs_lock to prevent list corruption against concurrent calls
to vfio_ap_mdev_probe()?

[ ... ]

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