Re: [PATCH] s390/vfio-ap: Fix control domain removal 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] Missing NULL check on `matrix_mdev->kvm` before acquiring its lock on AP configuration change paths, leading to a kernel panic if a mediated device is unattached.
- [High] Guest APCB is not updated when the last adapter or domain is removed, leaving the guest with unauthorized hardware access.
- [High] Iterating `mdev_list` without the appropriate mutex allows concurrent list modifications, leading to list corruption or use-after-free.
--

--- Patch [1]: [PATCH] s390/vfio-ap: Fix control domain removal in vfio_ap_mdev_cfg_remove ---
commit 934a77df4b71bb3ee940f94ad6abdd8290e96d28
Author: Anthony Krowiak <[email protected]>
Subject: s390/vfio-ap: Fix control domain removal in vfio_ap_mdev_cfg_remove

This commit modifies the vfio_ap_config_remove function to use bitmap_and()
instead of bitmap_andnot() when clearing bits from matrix_mdev->matrix.adm.
This ensures that explicitly unplugged control domains are correctly removed
from the KVM guest.

> diff --git a/drivers/s390/crypto/vfio_ap_ops.c b/drivers/s390/crypto/vfio_ap_ops.c
> index 44b3a1dcc1b36..1bfdbe85023dd 100644
> --- a/drivers/s390/crypto/vfio_ap_ops.c
> +++ b/drivers/s390/crypto/vfio_ap_ops.c
> @@ -2615,9 +2615,9 @@ static void vfio_ap_mdev_cfg_remove(unsigned long *ap_remove,

[Severity: High]
This is a pre-existing issue, but does vfio_ap_mdev_cfg_remove() safely handle
unattached mediated devices?

If an mdev is created via sysfs, matrix_mdev->kvm is initialized to NULL. If a
host AP configuration change occurs before this mdev is attached to a guest,
the loop in this function:

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

will unconditionally dereference the NULL kvm pointer. Is there a missing NULL
check for matrix_mdev->kvm before acquiring its lock on AP configuration change
paths?

[Severity: High]
This isn't a bug introduced by this patch, but can the iteration over
mdev_list in vfio_ap_mdev_cfg_remove() race with mdev creation?

vfio_ap_mdev_probe() adds a new mdev to matrix_dev->mdev_list using list_add()
while holding only matrix_dev->mdevs_lock:

drivers/s390/crypto/vfio_ap_ops.c: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);
    ...
}

However, vfio_ap_mdev_cfg_remove() iterates over mdev_list using
list_for_each_entry() holding only matrix_dev->guests_lock, as mdevs_lock is
only acquired individually inside the loop body.

Could this lead to kernel memory corruption or use-after-free if a user
creates a new mdev concurrently with an asynchronous host AP configuration
change?

>  		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 vfio_ap_mdev_hot_unplug_cfg() properly
update the guest APCB when the last device is removed?

In vfio_ap_mdev_hot_unplug_cfg(), the decision to update the guest's APCB
relies on bitmap_andnot():

drivers/s390/crypto/vfio_ap_ops.c:vfio_ap_mdev_hot_unplug_cfg() {
    ...
    if (!bitmap_empty(cdrem, AP_DOMAINS))
        do_hotplug |= bitmap_andnot(matrix_mdev->shadow_apcb.adm,
                                    matrix_mdev->shadow_apcb.adm,
                                    cdrem, AP_DOMAINS);
    ...
}

The kernel's bitmap_andnot() function returns true only if the resulting
destination bitmap is non-empty. If the last adapter, domain, or control domain
is explicitly removed, the destination shadow APCB becomes entirely empty,
causing bitmap_andnot() to return false.

Since do_hotplug remains false, vfio_ap_mdev_update_guest_apcb() is skipped.
Does this leave KVM guests with unauthorized access to physical hardware devices
that have been explicitly removed from the host's AP configuration?

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