Re: [PATCH v3 5/9] s390/vfio-ap: Fix control domain removal in vfio_ap_mdev_cfg_remove

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

Pre-existing issues:
- [High] Unplugging the last AP adapter, domain, or control domain fails to update the KVM guest's APCB.
--

--- Patch [5]: [PATCH v3 5/9] s390/vfio-ap: Fix control domain removal in vfio_ap_mdev_cfg_remove ---
commit ac232781abc6c5d395fdb856a5975b162da68506
Author: Anthony Krowiak <[email protected]>

s390/vfio-ap: Fix control domain removal in vfio_ap_mdev_cfg_remove

This commit resolves an issue in vfio_ap_mdev_cfg_remove where the use of
bitmap_andnot prevented explicitly unplugged control domains from being
removed from the KVM guest. The fix correctly changes the bitwise operation
to use bitmap_and instead.

Cc: [email protected]

> diff --git a/drivers/s390/crypto/vfio_ap_ops.c b/drivers/s390/crypto/vfio_ap_ops.c
> index 8a1ecb8915044..b6d0c988cc810 100644
> --- a/drivers/s390/crypto/vfio_ap_ops.c
> +++ b/drivers/s390/crypto/vfio_ap_ops.c
> @@ -2625,9 +2625,9 @@ static void vfio_ap_mdev_cfg_remove(unsigned long *ap_remove,
>  		do_remove |= bitmap_and(aqrem, aq_remove,
>  					  matrix_mdev->matrix.aqm,
>  					  AP_DOMAINS);
> -		do_remove |= bitmap_andnot(cdrem, cd_remove,
> -					     matrix_mdev->matrix.adm,
> -					     AP_DOMAINS);
> +		do_remove |= bitmap_and(cdrem, cd_remove,
> +					matrix_mdev->matrix.adm,
> +					AP_DOMAINS);
>  
>  		if (do_remove)
>  			vfio_ap_mdev_hot_unplug_cfg(matrix_mdev, aprem, aqrem,

[Severity: High]
This is a pre-existing issue, but does this function call successfully
update the KVM guest APCB when the very last hardware resource is
unplugged?

Looking inside vfio_ap_mdev_hot_unplug_cfg(), it uses the return value of
bitmap_andnot() to determine if an update is required:

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);
	}
    ...
	if (do_hotplug)
		vfio_ap_mdev_update_guest_apcb(matrix_mdev);
}

Because bitmap_andnot() returns false when the resulting bitmap is empty,
clearing the final bit from the shadow APCB returns 0.

If the last AP adapter, domain, or control domain is unplugged, do_hotplug
evaluates to 0 and the vfio_ap_mdev_update_guest_apcb() call is completely
bypassed.

Could this allow a guest to retain architectural access to hardware that has
been explicitly removed and potentially reassigned to another driver?

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