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

Pre-existing issues:
- [High] Unchecked dereference of `matrix_mdev->kvm` in `vfio_ap_mdev_cfg_remove` and `vfio_ap_mdev_hot_plug_cfg` leading to a kernel panic for mediated devices not attached to a KVM guest.
- [High] Control domain hot unplug in `vfio_ap_mdev_cfg_remove()` is completely broken due to incorrect use of `bitmap_andnot` instead of `bitmap_and`.
- [High] Unsafe iteration over `matrix_dev->mdev_list` in `vfio_ap_mdev_cfg_remove` without holding `matrix_dev->mdevs_lock`, risking a data race.
- [High] Unsupported older AP adapters (pre-CEX4) can bypass filtering in `vfio_ap_mdev_cfg_add` if no new domains are added simultaneously.
--

--- Patch [1]: [PATCH v4 1/9] s390/vfio-ap: Fix stale do_remove flag across iterations in vfio_ap_mdev_cfg_remove ---
commit 842cc8fca6dcd2512ea1bac051db5a235987ced0
Author: Anthony Krowiak <[email protected]>

s390/vfio-ap: Fix stale do_remove flag across iterations in vfio_ap_mdev_cfg_remove

This commit fixes the initialization and accumulation of the do_remove flag
in vfio_ap_mdev_cfg_remove(). Previously, the flag was never reset per
iteration, causing a positive result to carry over to subsequent matrix mdevs.
The fix sets the flag using bitmap_and() for the first check rather than
accumulating it.

> 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 is it safe to iterate over
matrix_dev->mdev_list here without holding matrix_dev->mdevs_lock?

The caller vfio_ap_on_cfg_changed() holds matrix_dev->guests_lock, but not
matrix_dev->mdevs_lock. Since mdevs_lock is acquired inside the loop and
released before the next iteration, concurrent modifications might corrupt
the list traversal if new mdevs are added in vfio_ap_mdev_probe() holding
only mdevs_lock.

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

[Severity: High]
This is a pre-existing issue, but does this code unconditionally dereference
matrix_mdev->kvm without checking for null?

If a mediated device is created but not yet attached to a kvm guest,
matrix_mdev->kvm remains initialized to null. If a host ap configuration
change occurs in this state, wouldn't this cause a kernel panic?

The same unconditional dereference appears to exist in
vfio_ap_mdev_hot_plug_cfg() when called from vfio_ap_on_cfg_changed().

>  		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 isn't a bug introduced by this patch, but looking at the next lines in
vfio_ap_mdev_cfg_remove():

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);
    ...
}

Is it correct to use bitmap_andnot() here instead of bitmap_and()?

This computes cd_remove & ~matrix_mdev->matrix.adm, yielding domains that
were removed from the host but are explicitly not assigned to the mdev.
Later, vfio_ap_mdev_hot_unplug_cfg() checks for intersection between the
guest's active domains and cdrem, which will always fail. Should this be
bitmap_and() instead?

[Severity: High]
This isn't a bug introduced by this patch, but is it possible for unsupported
older ap adapters to bypass filtering?

In vfio_ap_mdev_cfg_add(), the code calls:

drivers/s390/crypto/vfio_ap_ops.c:vfio_ap_mdev_cfg_add() {
    ...
    vfio_ap_filter_apid_by_qtype(apm_add, aqm_add);
    ...
}

If an older adapter is added to the host configuration without any
new domains being added simultaneously (because the domains were already
present), aqm_add will be empty.

This causes the inner loop in vfio_ap_filter_apid_by_qtype() to be skipped:

drivers/s390/crypto/vfio_ap_ops.c:vfio_ap_filter_apid_by_qtype() {
    ...
    for_each_set_bit_inv(apqi, aqm, AP_DOMAINS) {
        status = ap_test_queue(AP_MKQID(apid, apqi), 1, &info);
    ...
}

Because aqm is empty, the loop evaluating adapter type does not execute,
meaning the adapter's type is never verified and it remains in apm_add.
Should the active domains be passed to the filter instead of the delta?

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