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

On 8/12/26 8:12 AM, [email protected] wrote:
> 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?

Will add a block comment here to point out that it is safe to iterate 
the list
because vfio_ap_on_cfg_changed() - which is called prior to this - holds the
guests_lock. Hopefully that stops this from being pointed out by LLMs. Will
also add a comment prior to adding a matrix_mdev in the vfio_ap_mdev_probe
function for the same reason.

>
>> -		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()?

Fixed in patch 4/9 of this series.

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

Fixed in patch 6/9 of this series.

>
> [ ... ]
>>   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()?

No, the matrix_dev->guests_lock is needed to prevent list corruption.

>
> [ ... ]
>