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

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

[Anthony Krowiak]

The vfio_ap_config_remove function uses the bitmap_andnot function to clear
bits from the matrix_mdev->matrix.adm (specifies the control domains
assigned to the mdev). This prevents the explicitly unplugged control
domains from being removed the KVM guest. The bitmap_and function is used
instead.

Fixes: eeb386aeb5b7c ("s390/vfio-ap: handle config changed and scan complete notification")
Cc: stable@vger.kernel.org
Signed-off-by: Anthony Krowiak <akrowiak@linux.ibm.com>
---
 drivers/s390/crypto/vfio_ap_ops.c | 6 +++---
 1 file changed, 3 insertions(+), 3 deletions(-)

diff --git a/drivers/s390/crypto/vfio_ap_ops.c b/drivers/s390/crypto/vfio_ap_ops.c
index 44b3a1dcc1b3..1bfdbe85023d 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,
 		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,
-- 
2.53.0

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

[sashiko-bot]

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 <akrowiak@linux.ibm.com>
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/20260807223939.573718-1-akrowiak@linux.ibm.com?part=1

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

[Anthony Krowiak]

On 8/7/26 6:58 PM, sashiko-bot@kernel.org wrote:
> 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 <akrowiak@linux.ibm.com>
> 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?

A patch has already been posted for this:
https://lkml.org/lkml/2026/8/7/1453

I think it would be wise to submit that patch and this as a series since
both bugs were introduced with the same patch, so the Fixes: tag
for each would be the same.

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

This needs to be fixed.

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

The logic here is correct. The
concern rests on a misreading of what bitmap_andnot() returns
and what the bitmaps represent.

There are three functions that form a pipeline:

vfio_ap_mdev_on_cfg_remove() computes which adapters,
domains and control domains have been removed from the host
AP configuration by comparing the previous and current config
info bitmaps. If nothing was removed from a given bitmap,
that bitmap is empty and there is nothing to do.

vfio_ap_mdev_cfg_remove() intersects each of those
removed-from-host bitmaps against matrix_mdev->matrix,
which represents what the administrator has assigned to the
mdev. If the intersection is empty — none of the host-removed
resources were assigned to this mdev — there is nothing to
hot-unplug for this mdev.

vfio_ap_mdev_hot_unplug_cfg() intersects those
mdev-assigned-and-host-removed bitmaps against
matrix_mdev->shadow_apcb, which represents what is actually
passed through to the guest after filtering. The shadow_apcb
is a filtered subset of matrix — a resource assigned to the
mdev may not be in the shadow APCB if it was previously
filtered out (e.g., the queue was not passable, or the
resource was not in the host config at the time of the last
filter pass). If bitmap_andnot() returns false here, it
means none of the removed resources were present in the shadow
APCB to begin with — the guest never had access to them — so
skipping vfio_ap_mdev_update_guest_apcb() is correct.

Furthermore, bitmap_andnot(dst, src, rem) returns false only
when the result dst = src & ~rem is all zeros. If none of the
rem bits are present in src, then ~rem clears nothing and
dst == src. If src was non-empty, the return value is true
and vfio_ap_mdev_update_guest_apcb() is called — even though
the APCB content is unchanged. So there is no path through this
function where a genuine removal from the guest's APCB is
silently dropped.

>