Re: [PATCH v4 4/9] s390/vfio-ap: Fix required lock not held during update of ap_matrix_mdev object

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

Pre-existing issues:
- [High] Hardware validation is skipped when an adapter is hotplugged without new domains, allowing unsupported hardware to be assigned.
--

--- Patch [4]: [PATCH v4 4/9] s390/vfio-ap: Fix required lock not held during update of ap_matrix_mdev object ---
commit 6a90ea8f2f04defd282ac1432c8f5ff791b149f2
Author: Anthony Krowiak <[email protected]>

s390/vfio-ap: Fix required lock not held during update of ap_matrix_mdev object

This patch resolves a missing lock issue in vfio_ap_mdev_cfg_add(). It ensures
that the mdevs_lock is held while modifying the apm_add, aqm_add, and adm_add
bitmap fields. This prevents race conditions with concurrent modifications to
the matrix device.

> diff --git a/drivers/s390/crypto/vfio_ap_ops.c b/drivers/s390/crypto/vfio_ap_ops.c
> index f2d662e388bdc..21c502598f8ca 100644
> --- a/drivers/s390/crypto/vfio_ap_ops.c
> +++ b/drivers/s390/crypto/vfio_ap_ops.c
> @@ -2777,12 +2777,20 @@ static void vfio_ap_mdev_cfg_add(unsigned long *apm_add, unsigned long *aqm_add,

[Severity: High]
This isn't a bug introduced by this patch, but does 
vfio_ap_filter_apid_by_qtype() skip hardware validation if an adapter is 
hotplugged without new domains?

In vfio_ap_filter_apid_by_qtype(), if aqm is empty:

drivers/s390/crypto/vfio_ap_ops.c:vfio_ap_filter_apid_by_qtype() {
...
    for_each_set_bit_inv(apid, apm, AP_DEVICES) {
        apid_cleared = false;

        for_each_set_bit_inv(apqi, aqm, AP_DOMAINS) {
            status = ap_test_queue(AP_MKQID(apid, apqi), 1, &info);
...
}

The inner loop iterating over aqm (which maps to aqm_add) will not execute
if no domains were added during the config change. As a result, the hardware
is never queried with ap_test_queue(), and unsupported older adapters (older
than CEX4) are not filtered out from apm_add.

Can this allow unsupported hardware to be assigned to a KVM guest if a host
administrator hotplugs an older AP adapter without simultaneously hotplugging
new domains?

>  	 * called.
>  	 */
>  	list_for_each_entry(matrix_mdev, &matrix_dev->mdev_list, node) {
> +		/*
> +		 * The mdevs_lock must be held in order to access fields
> +		 * within matrix_mdev
> +		 */
> +		mutex_lock(&matrix_dev->mdevs_lock);
> +
>  		bitmap_and(matrix_mdev->apm_add,
>  			   matrix_mdev->matrix.apm, apm_add, AP_DEVICES);

[ ... ]

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