Re: [PATCH 1/5] s390/vfio-ap: Fix stale do_remove flag across iterations in vfio_ap_mdev_cfg_remove

On 8/8/26 1:49 PM, [email protected] wrote:
> Thank you for your contribution! Sashiko AI review found 4 potential issue(s) to consider:
>
> Pre-existing issues:
> - [Critical] Unconditional dereference of `matrix_mdev->kvm` pointer leading to host kernel panic.
> - [High] Concurrent modification and traversal of `matrix_dev->mdev_list` without consistent locking.
> - [High] Assigned control domains removed from the host are not unplugged from the KVM guest due to incorrect bitmap calculation.
> - [High] Unplugging the last assigned AP resource from an mdev fails to update the KVM guest configuration.
> --
>
> --- Patch [1]: [PATCH 1/5] s390/vfio-ap: Fix stale do_remove flag across iterations in vfio_ap_mdev_cfg_remove ---
> commit bb84df1768da7fbde04fb6494c22c41ed756bd46
> Author: Anthony Krowiak <[email protected]>
>
> s390/vfio-ap: Fix stale do_remove flag across iterations in vfio_ap_mdev_cfg_remove
>
> This commit fixes an issue in vfio_ap_mdev_cfg_remove() where the do_remove
> flag is incorrectly accumulated across iterations. It ensures the flag is
> properly assigned on the first bitmap operation for each mdev.
>
>> diff --git a/drivers/s390/crypto/vfio_ap_ops.c b/drivers/s390/crypto/vfio_ap_ops.c
>> index 44b3a1dcc1b36..845c86ba8bc30 100644
>> --- a/drivers/s390/crypto/vfio_ap_ops.c
>> +++ b/drivers/s390/crypto/vfio_ap_ops.c
>> @@ -2603,15 +2603,15 @@ static void vfio_ap_mdev_cfg_remove(unsigned long *ap_remove,
>>   	DECLARE_BITMAP(aprem, AP_DEVICES);
>>   	DECLARE_BITMAP(aqrem, AP_DOMAINS);
>>   	DECLARE_BITMAP(cdrem, AP_DOMAINS);
>> -	int do_remove = 0;
>> +	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 does this code risk list
> corruption or following invalid pointers?
>
> The list is being iterated without holding mdevs_lock across the entire loop
> (the lock is acquired and released inside the loop). Concurrently,
> vfio_ap_mdev_probe() adds new mdevs to the list while holding only mdevs_lock:
>
> drivers/s390/crypto/vfio_ap_ops.c:vfio_ap_mdev_probe() {
>      ...
>      list_add(&matrix_mdev->node, &matrix_dev->mdev_list);
>      ...
> }
>
> Could this concurrent modification and traversal crash the host kernel?

No. The call stack for this function is:
vfio_ap_on_cfg_changed -> vfio_ap_mdev_on_cfg_remove -> 
vfio_ap_mdev_config_remove

In vfio_ap_cfg_changed, the matrix_dev->guests_lock mutex is taken and 
held for the duration
of the function. The guests_lock guards access to the 
matrix_dev->mdev_list. All functions
that add or remove ap_matrix_mdev objects also take this mutex, so there 
should never be
concurrent modification of the list.

>
>>   		mutex_lock(&matrix_mdev->kvm->lock);
> [Severity: Critical]
> This is a pre-existing issue, but can this unconditionally dereference a null
> pointer?

It can; however, the problem is fixed with patch 02/05 in the patch 
series in which this patch is
included.

>
> When an mdev is created, its kvm pointer is null until it is attached to a
> guest. If a host AP configuration change occurs while an unattached mdev
> exists, this unconditionally executes mutex_lock(&matrix_mdev->kvm->lock),
> which dereferences the null kvm pointer.
>
> Could this lead to a host kernel panic?
>
>>   		mutex_lock(&matrix_dev->mdevs_lock);
>>   
>> -		do_remove |= bitmap_and(aprem, ap_remove,
>> -					  matrix_mdev->matrix.apm,
>> -					  AP_DEVICES);
>> +		do_remove = bitmap_and(aprem, ap_remove,
>> +				       matrix_mdev->matrix.apm,
>> +				       AP_DEVICES);
>>   		do_remove |= bitmap_and(aqrem, aq_remove,
>>   					  matrix_mdev->matrix.aqm,
>>   					  AP_DOMAINS);
> [Severity: High]
> This is a pre-existing issue in vfio_ap_mdev_cfg_remove(), but is this bitmap
> calculation correct?
>
> Right below the quoted code, cdrem is calculated:
>
> drivers/s390/crypto/vfio_ap_ops.c:vfio_ap_mdev_cfg_remove() {
>      ...
> 		do_remove |= bitmap_andnot(cdrem, cd_remove,
> 					     matrix_mdev->matrix.adm,
> 					     AP_DOMAINS);
>      ...
> }
>
> This evaluates to cd_remove & ~(matrix.adm), which yields domains removed from
> the host that are explicitly not assigned to the mdev. Assigned domains that
> are removed from the host are omitted and therefore not hot-unplugged.
>
> Could this allow a control domain removed from the host AP configuration to
> remain improperly assigned and accessible to the KVM guest?

This problem is fixed with patch 5/5 in the patch series in which this 
patch is
included.

>
> [Severity: High]
> This is another pre-existing issue, but does unplugging the last assigned
> AP resource from an mdev fail to update the KVM guest configuration?
>
> In vfio_ap_mdev_hot_unplug_cfg(), do_hotplug is determined using bitmap_andnot:
>
> drivers/s390/crypto/vfio_ap_ops.c:vfio_ap_mdev_hot_unplug_cfg() {
>      ...
> 	if (!bitmap_empty(aprem, AP_DEVICES)) {
> 		do_hotplug |= bitmap_andnot(matrix_mdev->shadow_apcb.apm,
> 					    matrix_mdev->shadow_apcb.apm,
> 					    aprem, AP_DEVICES);
> 	}
>      ...
> }
>
> bitmap_andnot returns true only if the resulting destination bitmap is
> non-empty. If the last adapter or domain is removed, the resulting mask is
> empty, bitmap_andnot returns false, and do_hotplug remains false.
>
> Would this skip the APCB update, leaving the KVM guest with its old KVM CRYCBD
> configuration and retaining access to an AP resource that was successfully
> removed from the host?

The logic here is correct. aprem is a bitmap specifying the adapters 
that have
been removed from the host's AP configuration. The bitmap_andnot will return
true only if the matrix_mdev->shadow_apcb and aprem and therefore the
intersecting bits have been removed from shadow_apcb. If there is not
intersection, then no bits will have been removed and the bitmap_andnot
will return 0, in which case there is no need to make changes on the guest.

>