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

[Anthony Krowiak <akrowiak@linux.ibm.com>]

On 8/10/26 10:01 AM, Anthony Krowiak wrote:
>
>
> On 8/8/26 1:49 PM, sashiko-bot@kernel.org 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 <akrowiak@linux.ibm.com>
>>
>> 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.

I am wrong here; a misunderstanding of the return value of 
bitmap_andnot. See my
response to patch 5/5 which mentions this same thing.


>
>>
>