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

Thank you for your contribution! Sashiko AI review found 5 potential issue(s) to consider:

Pre-existing issues:
- [High] Data race due to iterating `mdev_list` without holding `mdevs_lock` across the entire loop.
- [High] Unconditional dereference of `matrix_mdev->kvm` when acquiring the KVM lock.
- [High] Incorrect use of `bitmap_andnot` instead of `bitmap_and` for calculating `cdrem` prevents control domains from being hot-unplugged.
- [High] Misuse of `bitmap_andnot` return value causes missed and spurious hotplug events in `vfio_ap_mdev_hot_unplug_cfg`.
- [Medium] Incorrect bitmap size constants (`AP_DEVICES` vs `AP_DOMAINS`) passed to bitmap operations.
--

--- Patch [1]: [PATCH v3 1/9] s390/vfio-ap: Fix stale do_remove flag across iterations in vfio_ap_mdev_cfg_remove ---
commit 9ff3dc11652e82272549805f96671e76c090b814
Author: Anthony Krowiak <[email protected]>

s390/vfio-ap: Fix stale do_remove flag across iterations in vfio_ap_mdev_cfg_remove

The do_remove flag in vfio_ap_mdev_cfg_remove() is initialized to zero but
never reset at the start of each iteration. Because do_remove is accumulated
with bitwise OR across iterations, a positive result from one mdev carries
over to subsequent mdevs. This commit fixes the issue by setting the
do_remove flag directly with the first call to bitmap_and.

> 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 is a pre-existing issue, but does iterating over mdev_list here while
repeatedly dropping and reacquiring mdevs_lock inside the loop create a data
race?

Because mdevs_lock is not held when the iterator advances, couldn't a
concurrent call to vfio_ap_mdev_probe() modify mdev_list, leading to
list corruption or use-after-free?

vfio_ap_mdev_probe() adds to this list holding only mdevs_lock:

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

>  		mutex_lock(&matrix_mdev->kvm->lock);

[Severity: High]
This isn't a bug introduced by this patch, but can matrix_mdev->kvm be NULL
here?

When an mdev is created via sysfs, matrix_mdev->kvm appears to be initialized
to NULL and is only assigned when the mdev is opened by KVM. If a host AP
configuration change occurs before the mdev is attached to a guest, couldn't
this unconditional dereference cause a host kernel panic?

(Note: similar unconditional dereferences appear in
vfio_ap_mdev_hot_plug_cfg)

>  		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, but does the calculation for cdrem just below
this diff use the wrong bitwise operation?

    do_remove |= bitmap_andnot(cdrem, cd_remove,
                               matrix_mdev->matrix.adm,
                               AP_DOMAINS);

By using bitmap_andnot (which computes cd_remove & ~matrix_mdev->matrix.adm),
isn't this incorrectly identifying domains removed from the host that are not
assigned to this guest? Shouldn't this be an intersection (bitmap_and) to
identify assigned domains being removed, just like the calculations for aprem
and aqrem?


[Severity: High]
This isn't a bug introduced by this patch, but is the return value of
bitmap_andnot() being misused in 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);
    }

The standard bitmap_andnot() function returns true if the resulting
destination bitmap is non-empty, not whether bits were actually changed.

If the last device is removed from the guest's shadow APCB, the resulting
bitmap is empty, so bitmap_andnot() returns false and the guest is never
notified. Conversely, if a device is removed that wasn't in the shadow APCB
but the guest has other devices, it returns true, triggering a spurious
update.


[Severity: Medium]
This is a pre-existing issue, but are the wrong size constants being passed to
bitmap operations for domains?

In vfio_ap_mdev_hot_unplug_cfg():

    do_hotplug |= bitmap_andnot(matrix_mdev->shadow_apcb.aqm,
                                matrix_mdev->shadow_apcb.aqm,
                                aqrem, AP_DEVICES);

And in vfio_ap_mdev_cfg_add():

    bitmap_and(matrix_mdev->adm_add,
               matrix_mdev->matrix.adm, adm_add, AP_DEVICES);

Since aqm and adm represent domains, shouldn't these use AP_DOMAINS instead
of AP_DEVICES? This could lead to out-of-bounds memory accesses if the
definitions of AP_DEVICES and AP_DOMAINS ever diverge.

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