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

Pre-existing issues:
- [High] Hot-plug add state is overwritten instead of accumulated, potentially dropping hot-plug events.
- [High] Pinned page and GISC registration leak in vfio_ap_irq_enable() when ap_aqic() returns a default error code.
--

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

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

This commit adds the missing matrix_dev->mdevs_lock in vfio_ap_mdev_cfg_add()
to guard against race conditions when modifying the ap_matrix_mdev object's
fields.

> 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,
>  	 * 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);
>  		bitmap_and(matrix_mdev->aqm_add,
>  			   matrix_mdev->matrix.aqm, aqm_add, AP_DOMAINS);
>  		bitmap_and(matrix_mdev->adm_add,
>  			   matrix_mdev->matrix.adm, adm_add, AP_DEVICES);

[Severity: High]
This isn't a bug introduced by this patch, but does this overwrite the
hot-plug addition state instead of accumulating it?

If multiple AP configuration change events occur before the AP bus scan
completes, it appears that vfio_ap_mdev_cfg_add() unconditionally overwrites
the deferred hot-plug bitmaps with the delta of the most recent event.

Should this logic use a temporary intersection followed by bitmap_or() to
accumulate the pending additions so that hot-plug events aren't dropped?

> +
> +		mutex_unlock(&matrix_dev->mdevs_lock);
>  	}
>  }

While reviewing the file, I noticed a pre-existing issue in
vfio_ap_irq_enable():

> drivers/s390/crypto/vfio_ap_ops.c:vfio_ap_irq_enable() {
> 	...
> 	default:
> 		pr_warn("%s: apqn %04x: response: %02x\n", __func__, q->apqn,
> 			status.response_code);
> 		vfio_ap_irq_disable(q);
> 		break;
> 	}
> 	...
> }

[Severity: High]
This is a pre-existing issue, but does this default error path leak the
newly pinned nib and registered nisc?

When ap_aqic() returns an unexpected error falling into the default case,
vfio_ap_irq_disable() is called. However, it seems vfio_ap_irq_disable()
only cleans up the previously successfully saved configuration, bypassing
the newly pinned nib and isc held in local variables.

Can a malicious KVM guest repeatedly issue the PQAP(AQIC) instruction with
configurations that trigger this error path, causing unbounded memory pinning
on the host?

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