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

[sashiko-bot@kernel.org]

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?

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

[Severity: Critical]
This is a pre-existing issue, but can this unconditionally dereference a null
pointer?

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?

[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?

-- 
Sashiko AI review · https://sashiko.dev/#/patchset/20260808170712.839066-1-akrowiak@linux.ibm.com?part=1