Re: [PATCH v4 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] Concurrent modification and traversal of the `matrix_dev->mdev_list` without a shared lock leads to list corruption.
- [High] Uninitialized stack variable `apm_filtered` causes arbitrary queue resets.
- [High] Use-after-free on `kvm->lock` inside `vfio_ap_mdev_unset_kvm()`.
--

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

This commit fixes a potential NULL pointer dereference in the callback
functions vfio_ap_on_cfg_changed and vfio_ap_on_scan_complete. It adds
checks to ensure matrix_mdev->kvm is not NULL before acquiring the
matrix_mdev->kvm->lock mutex. The lock acquisition in
vfio_ap_mdev_hot_plug_cfg is also moved to the calling functions to
satisfy lock ordering requirements.

> 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
> @@ -2605,8 +2605,20 @@ static void vfio_ap_mdev_cfg_remove(unsigned long *ap_remove,
>  	DECLARE_BITMAP(cdrem, AP_DOMAINS);
>  	int do_remove;
>  
> +	/*
> +	 * It is safe to traverse this list here because the
> +	 * required guard - matrix_dev->guests_lock - is taken in the
> +	 * vfio_ap_on_cfg_changed function prior to this function getting
> +	 * called.
> +	 */
>  	list_for_each_entry(matrix_mdev, &matrix_dev->mdev_list, node) {

[Severity: High]
This isn't a bug introduced by this patch, but does the traversal of
matrix_dev->mdev_list here race with list modifications?

In vfio_ap_mdev_probe(), a newly created mediated device is added to the list
while holding only mdevs_lock:

	mutex_lock(&matrix_dev->mdevs_lock);
	list_add(&matrix_mdev->node, &matrix_dev->mdev_list);
	mutex_unlock(&matrix_dev->mdevs_lock);

Here in vfio_ap_mdev_cfg_remove() (and similarly in 
vfio_ap_on_scan_complete()), the list is traversed while holding only
guests_lock. Since the writer and readers do not acquire a shared lock, can
this concurrent execution lead to a corrupted doubly linked list?

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

[ ... ]

> @@ -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;
>  

[Severity: High]
This is a pre-existing issue, but could apm_filtered remain uninitialized
before being consumed?

If a host AP configuration change adds only control domains, filter_adapters
and filter_domains will evaluate to false:

	filter_adapters = bitmap_intersects(matrix_mdev->matrix.apm,
					    matrix_mdev->apm_add, AP_DEVICES);
	filter_domains = bitmap_intersects(matrix_mdev->matrix.aqm,
					   matrix_mdev->aqm_add, AP_DOMAINS);

This causes the code to skip calling vfio_ap_mdev_filter_matrix(), which
normally initializes apm_filtered.

The uninitialized stack variable is then passed to reset_queues_for_apids():

	reset_queues_for_apids(matrix_mdev, apm_filtered);

Does this cause reset_queues_for_apids() to iterate over random stack garbage
and reset unintended queues?

> -	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,

[ ... ]

Additionally, there appears to be another pre-existing issue in
vfio_ap_mdev_unset_kvm() which isn't part of this diff:

[Severity: High]
Is there a potential use-after-free on kvm->lock in vfio_ap_mdev_unset_kvm()?

In vfio_ap_mdev_unset_kvm(), the code acquires kvm->lock via 
get_update_locks_for_kvm(kvm) and later drops the KVM reference:

	get_update_locks_for_kvm(kvm);
	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);

If userspace has already closed the KVM VM file descriptor, kvm_put_kvm()
could drop the final reference and free the kvm structure.

Immediately after this, release_update_locks_for_kvm(kvm) attempts to unlock
the mutex residing in the freed memory:

	if (kvm)
		mutex_unlock(&kvm->lock);

Could this result in a use-after-free on kvm->lock?

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