Re: [PATCH v6 11/15] s390/vfio-ap: Transition device migration state to STOP

[sashiko-bot@kernel.org]

Thank you for your contribution! Sashiko AI review found 4 potential issue(s) to consider:
- [Critical] Dropping mdevs_lock during copy_to_user and copy_from_user in migration data transfers allows concurrent device reset or close to free the ap_config buffer, leading to an exploitable Use-After-Free.
- [Critical] A double fetch of the userspace configuration buffer allows a malicious user to modify the num_queues field after size allocation, leading to an Out-Of-Bounds (OOB) heap read and unauthorized hardware access.
- [High] Permanent leak of migration file streams, ap_config buffers, and VFIO device registration references due to missing fput() when transitioning to the STOP state.
- [High] The mdevs_lock is held while calling ap_tapq(), a slow hardware instruction, violating the stated subsystem locking rules and risking host stalls.
--

--- Patch [11]: [PATCH v6 11/15] s390/vfio-ap: Transition device migration state to STOP ---
commit bf4a9d5f155b1c98e5568129336b12baad5821b5
Author: Anthony Krowiak <akrowiak@linux.ibm.com>

s390/vfio-ap: Transition device migration state to STOP

This commit updates the device migration state machine for vfio-ap to properly
handle the transition to the STOP state. Since there is no physical direct
memory access device virtualized by vfio-ap, the state transition logic now
terminates the data transfer session by returning NULL.

> diff --git a/drivers/s390/crypto/vfio_ap_migration.c b/drivers/s390/crypto/vfio_ap_migration.c
> index c12ba82ec5277..fc4cb893f0f00 100644
> --- a/drivers/s390/crypto/vfio_ap_migration.c
> +++ b/drivers/s390/crypto/vfio_ap_migration.c
> @@ -1267,12 +1267,17 @@ vfio_ap_transition_to_state(struct ap_matrix_mdev *matrix_mdev,
>  		return filp;
>  	}
>  
> +	/*
> +	 * Terminates the data transfer session of the vfio-ap device state
> +	 * between the source and target hosts. Since the vfio-ap device does
> +	 * not virtualize a DMA device, there is no internal device state to
> +	 * incorporate into the vfio-ap device on the target.
> +	 */
>  	if ((cur_state == VFIO_DEVICE_STATE_RESUMING &&
>  	     new_state == VFIO_DEVICE_STATE_STOP) ||
>  	    (cur_state == VFIO_DEVICE_STATE_STOP_COPY &&
>  	     new_state == VFIO_DEVICE_STATE_STOP)) {
> -		/* TODO */
> -		return ERR_PTR(-EOPNOTSUPP);
> +		return NULL;

[Severity: High]
Does returning NULL here permanently leak the migration file streams and
associated data?

When the state transitions to STOP, vfio_ap_transition_to_state() returns
NULL without relinquishing the driver's extra file reference. The driver
previously takes this extra reference in vfio_ap_open_file_stream():

drivers/s390/crypto/vfio_ap_migration.c:vfio_ap_open_file_stream() {
    ...
	get_file(filp);

	return filp;
}

If this reference isn't dropped, wouldn't we leak memory (struct file,
ap_config buffers, anonymous inodes) and vfio device registration pins?
This regression might potentially lead to resource exhaustion or prevent
device unbinding.

>  	}
>  
>  	if ((cur_state == VFIO_DEVICE_STATE_STOP &&

[Severity: Critical]
Can dropping mdevs_lock during copy operations lead to an exploitable
use-after-free regression?

In both vfio_ap_stop_copy_read() and vfio_ap_resuming_write(), the
mdevs_lock is explicitly dropped while a pointer to the ap_config buffer
is passed to copy_to_user() or copy_from_user():

drivers/s390/crypto/vfio_ap_migration.c:vfio_ap_stop_copy_read() {
    ...
	mutex_unlock(&matrix_dev->mdevs_lock);

	if (copy_to_user(buf, (char *)mig_file->ap_config + read_pos, len))
    ...
}

Concurrently, if another thread issues a vfio device reset ioctl,
couldn't vfio_ap_reset_migration_state() acquire mdevs_lock and
synchronously free the ap_config buffers via vfio_ap_release_mig_files()?
The copy operations would then proceed using the freed heap pointer.

[Severity: Critical]
Does this code allow an out-of-bounds heap read due to a double fetch of
the userspace configuration buffer?

In vfio_ap_resuming_write(), allocate_ap_config() performs an initial
copy_from_user() to determine num_queues and allocate the struct:

drivers/s390/crypto/vfio_ap_migration.c:vfio_ap_resuming_write() {
    ...
		ret = allocate_ap_config(&ap_config, buf, len);
    ...
}

Immediately afterward, vfio_ap_resuming_write() performs a second
copy_from_user() using the same unvalidated userspace buffer pointer:

    ...
	if (copy_from_user((char *)ap_config + write_pos, buf, len)) {
    ...

If a malicious user modifies the num_queues field between these fetches,
could the artificially inflated count cause an out-of-bounds read in
setup_ap_matrix_from_ap_config():

drivers/s390/crypto/vfio_ap_migration.c:setup_ap_matrix_from_ap_config() {
    ...
	for (int i = 0; i < ap_config->num_queues; i++) {
		qinfo = ap_config->qinfo[i];
		apid = AP_QID_CARD(qinfo.apqn);
    ...
}

and use it to grant unauthorized hardware access to the guest?

[Severity: High]
Are we holding mdevs_lock while executing a slow hardware instruction?

The ap_tapq() instruction is called in a loop within
vfio_ap_store_queue_info(), which includes a comment stating it must be
called without mdevs_lock held:

drivers/s390/crypto/vfio_ap_migration.c:vfio_ap_store_queue_info() {
    ...
	/*
	 * ap_tapq() is a hardware instruction that may take time to complete.
	 * It must be called without mdevs_lock held to avoid blocking other
	 * mdevs. The apqn list was already snapshotted into ap_config->qinfo[]
	 * by the caller under the lock.
	 */
	for (num_queues = 0; num_queues < ap_config->num_queues; num_queues++) {
		ret = get_hardware_info_for_queue(mdev_name, &source_hwinfo,
						  ap_config->qinfo[num_queues].apqn);
    ...
}

However, the call chain vfio_ap_stop_copy_read() -> vfio_ap_get_config()
explicitly takes mdevs_lock, asserts it, and then invokes
vfio_ap_store_queue_info() directly while still holding the mutex:

drivers/s390/crypto/vfio_ap_migration.c:vfio_ap_stop_copy_read() {
    ...
	mutex_lock(&matrix_dev->mdevs_lock);
    ...
	if (!mig_file->ap_config) {
		ret = vfio_ap_get_config(matrix_mdev);
    ...
}

Could this regression stall other mdev operations on the host and cause a
potential denial of service?

-- 
Sashiko AI review · https://sashiko.dev/#/patchset/20260727173239.2420754-1-akrowiak@linux.ibm.com?part=11