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

On 8/12/26 4:18 PM, [email protected] wrote:
> 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?

Looking at ap_scan_bus(), the entire execution is strictly sequential and
single-threaded under ap_scan_bus_mutex:

1. ap_get_configuration() is called once — a single PQAP(QCI)
    captures the snapshot of the configuration delta for the entire scan.

2. If config_changed, notify_config_changed() is called once — this
     fires vfio_ap_on_cfg_changed -> vfio_ap_mdev_cfg_add exactly once 
per scan.

3. The adapter loop runs.

4. If config_changed, notify_scan_complete() is called once — this
     fires vfio_ap_on_scan_complete exactly once per scan.

Both notify_config_changed and notify_scan_complete are also guarded by
ap_scan_bus_mutex which ensures the entire callback pair is an
atomic unit from the perspective of any concurrent scan attempt.
The mutex_trylock(ap_scan_bus_mutex) is used, so a concurrent scan
request is simply dropped if one is already in progress, not queued 
behind it.

So the scenario the that two on_cfg_changed firings accumulating
in the bitmaps before on_scan_complete clears them is structurally 
impossible.
Each scan atomically fires on_cfg_changed (at most once) and 
on_scan_complete
(at most once) as an inseparable pair, with no mechanism for a second 
scan to
interleave. The comment is not valid.

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

This one is a real pre-existing bug. By the time the default case is
reached:

1. The nib has been pinned
2. The nisc has been registered

Neither has been saved to q->saved_iova / q->saved_isc yet, so when
vfio_ap_irq_disable(q) calls vfio_ap_free_aqic_resources(q), it releases
whatever is in q->saved_iova and q->saved_isc; the old resources from a
prior call. The newly pinned nib page and newly registered nisc are never
freed.

A fix patch is forthcoming.

>