Re: [PATCH v5 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 3 potential issue(s) to consider:

Pre-existing issues:
- [High] Unconditional dereference of `matrix_mdev->kvm` causes kernel panics for mdevs not yet attached to a guest.
- [High] Incorrect use of `bitmap_andnot` instead of `bitmap_and` prevents removed control domains from being hot-unplugged from guests.
- [High] Unprotected iteration of `matrix_dev->mdev_list` leads to potential data races and list corruption.
--

--- Patch [1]: [PATCH v5 1/9] s390/vfio-ap: Fix stale do_remove flag across iterations in vfio_ap_mdev_cfg_remove ---
commit 62f5d072f498c6d3b4f1ad645dbc6b551ae4f363
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 initialised to zero
before the loop that iterates over the list of matrix mdevs, but is
never reset at the start of each iteration. Since do_remove is
OR-accumulated across iterations, a positive result from one mdev
carries over to subsequent mdevs.

The fix is to set the do_remove flag with the first call to bitmap_and;
for example: do_remove = bitmap_an rather than do_remove |= 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 isn't a bug introduced by this patch, but does iterating
matrix_dev->mdev_list here require holding matrix_dev->mdevs_lock?

Looking at vfio_ap_mdev_probe(), it appears to add new mdevs to this list via
list_add() while holding only matrix_dev->mdevs_lock.

Since this loop is entered before acquiring matrix_dev->mdevs_lock, can an
mdev creation running concurrently with an AP configuration change modify the
list while it is being iterated, potentially leading to a data race or list
corruption?

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

[Severity: High]
This is a pre-existing issue, but could this unconditionally dereference a
NULL kvm pointer?

If a user creates a mediated device and assigns AP resources to it without
attaching it to a KVM guest, matrix_mdev->kvm would be NULL.

If a host AP configuration change occurs, would dereferencing matrix_mdev->kvm
here cause a kernel panic?

This same pattern appears in vfio_ap_mdev_hot_plug_cfg():

drivers/s390/crypto/vfio_ap_ops.c:vfio_ap_mdev_hot_plug_cfg() {
    ...
	list_for_each_entry(matrix_mdev, &matrix_dev->mdev_list, node) {
		mutex_lock(&matrix_mdev->kvm->lock);
    ...
}

Does this exhibit the same NULL pointer dereference risk when an mdev is not
yet attached to a guest?

>  		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 is bitmap_andnot() the correct operation for
calculating cdrem just below this?

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 computes the cdrem bitmask using inverted logic, isolating removed
domains that are NOT assigned to the mdev.

Should this use bitmap_and() like the aprem and aqrem calculations, so that
assigned control domains are properly identified and hot-unplugged from
guests?

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