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

On 8/7/26 11:18 AM, [email protected] wrote:
> Thank you for your contribution! Sashiko AI review found 1 potential issue(s) to consider:
>
> Pre-existing issues:
> - [High] Incorrect bitwise operation `bitmap_andnot` used instead of `bitmap_and` for control domains in `vfio_ap_mdev_cfg_remove`, preventing removal of unplugged domains.
> --
>
> --- Patch [1]: [PATCH] s390/vfio-ap: Fix dereference matrix_mdev->kvm without checking for NULL ---
> commit 7ae73def648a068715a8cdb4bf574ac5755f8e77
> Author: Anthony Krowiak <[email protected]>
>
> s390/vfio-ap: Fix dereference matrix_mdev->kvm without checking for NULL
>
> This commit adds NULL pointer checks for matrix_mdev->kvm prior to taking
> the kvm->lock mutex in the configuration change and scan complete callbacks.
> This prevents a NULL pointer dereference when the matrix mdev has no
> associated KVM instance.
>
> Cc: [email protected]
>
>> diff --git a/drivers/s390/crypto/vfio_ap_ops.c b/drivers/s390/crypto/vfio_ap_ops.c
>> index 44b3a1dcc1b36..a0b7c37fee281 100644
>> --- a/drivers/s390/crypto/vfio_ap_ops.c
>> +++ b/drivers/s390/crypto/vfio_ap_ops.c
>> @@ -2606,7 +2606,15 @@ static void vfio_ap_mdev_cfg_remove(unsigned long *ap_remove,
>>   	int do_remove = 0;
>>   
>>   	list_for_each_entry(matrix_mdev, &matrix_dev->mdev_list, node) {
>> -		mutex_lock(&matrix_mdev->kvm->lock);
>> +		/*
>> +		 * If the mdev is attached to a KVM guest, we will need to
>> +		 * hold the KVM lock in order to update the guest's AP
>> +		 * configuration if any adapters, domains or control domains
>> +		 * have been removed.
>> +		 */
>> +		if (matrix_mdev->kvm)
>> +			mutex_lock(&matrix_mdev->kvm->lock);
>> +
>>   		mutex_lock(&matrix_dev->mdevs_lock);
>>   
>>   		do_remove |= bitmap_and(aprem, ap_remove,
> [Severity: High]
> This is a pre-existing issue, but I noticed a potential logic flaw just
> below this context in vfio_ap_mdev_cfg_remove() within
> drivers/s390/crypto/vfio_ap_ops.c.
>
> Does the control domain removal incorrectly use bitmap_andnot() instead of
> bitmap_and()?
>
> When a host administrator or hypervisor unplugs a control domain, the code
> evaluates the removed domains using:
>
> 	do_remove |= bitmap_andnot(cdrem, cd_remove,
> 				   matrix_mdev->matrix.adm,
> 				   AP_DOMAINS);
>
> Since this evaluates to cd_remove & ~matrix_mdev->matrix.adm, doesn't this
> mean assigned domains that were removed from the host will resolve to 0?
>
> Could this prevent explicitly unplugged control domains from being removed
> from the KVM guest, allowing it to retain access to hardware it shouldn't
> have?

This looks like a cut and paste error from long ago. bitmap_and should have
been used as it was for the other bitmaps instead of bitmap_andnot.

>
> [ ... ]
>