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

[sashiko-bot@kernel.org]

Thank you for your contribution! Sashiko AI review found 8 potential issue(s) to consider:
- [Critical] Fragmented or malformed writes to the resuming file descriptor trigger severe memory corruption bugs (Double Free, NULL Dereference, Leak).
- [Critical] Insufficient length check in allocate_ap_config allows out-of-bounds heap read during post-copy validation.
- [Critical] Uninitialized stack variable `guest_matrix` is used to configure the guest AP matrix.
- [High] Improper handling of RESUMING -> STOP transition enables state machine bypass, lack of validation, and memory leaks.
- [High] The `config_sz` field in the serialized `struct vfio_ap_config` is never set during the STOP_COPY phase, breaking allocation logic on the target host.
- [High] Transitions between RUNNING and STOP states incorrectly return `-EOPNOTSUPP`.
- [High] Missing locking in `vfio_ap_release_mig_file` leads to a TOCTOU data race and Use-After-Free of migration state pointers.
- [Medium] `vfio_ap_release_migration_data` leaks `ap_config` when the device is closed.
--

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

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

This commit modifies the VFIO AP migration state machine to return NULL
when transitioning to the STOP state from RESUMING or STOP_COPY. It
removes the unsupported error return to allow the migration framework to
clean up the file streams.

> diff --git a/drivers/s390/crypto/vfio_ap_migration.c b/drivers/s390/crypto/vfio_ap_migration.c
> index 9cd1e7a3f7430..1b526b743ad02 100644
> --- a/drivers/s390/crypto/vfio_ap_migration.c
> +++ b/drivers/s390/crypto/vfio_ap_migration.c
> @@ -1159,12 +1159,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 without explicitly closing the file or clearing
mig_data->resuming_mig_state.filp allow a state machine bypass?

If a misbehaving userspace keeps the file descriptor open and transitions
the device back to RUNNING, it appears it could bypass migration state
machine constraints. Because vfio_ap_resuming_write() only checks that the
file descriptor matches resuming_mig_state.filp rather than verifying the
current mig_state, could userspace overwrite the active AP configuration
while the device is fully running?

Also, would initiating another migration cycle blindly overwrite filp and
cause a memory leak?

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

[Severity: High]
Are transitions between RUNNING and STOP states incorrectly returning
-EOPNOTSUPP in this adjacent block?

drivers/s390/crypto/vfio_ap_migration.c:vfio_ap_transition_to_state() {
	...
	if ((cur_state == VFIO_DEVICE_STATE_STOP &&
	     new_state == VFIO_DEVICE_STATE_RUNNING) ||
	    (cur_state == VFIO_DEVICE_STATE_RUNNING &&
	     new_state == VFIO_DEVICE_STATE_STOP)) {
		/* TODO */
		return ERR_PTR(-EOPNOTSUPP);
	}
	...
}

The VFIO migration specification mandates that devices must support
transitions between VFIO_DEVICE_STATE_RUNNING and VFIO_DEVICE_STATE_STOP.
When vfio_mig_get_next_state() attempts to transition the device through the
state machine, will this stub logic instantly fail and render the device
migration feature broken?

[Severity: Critical]
Can fragmented or malformed writes to the resuming file descriptor trigger
memory corruption?

In vfio_ap_resuming_write():

	if (*pos + len == cfg_sz) {
		ret = do_post_copy_validation(mdev_name, ap_config);
		...
		mig_data->resuming_mig_state.ap_config = ap_config;
	}

If a short write occurs where len < sizeof(config), it appears ap_config is
allocated locally but not saved to mig_data because *pos + len == cfg_sz
evaluates to false, causing a memory leak.

Then, if a subsequent write fetches the unsaved (NULL) pointer and passes it
to reallocate_ap_config():

	cur_cfg_sz = ap_config->config_sz;

Could this unconditionally dereference ap_config, causing a kernel crash?

Alternatively, if a full write is followed by an extra write,
reallocate_ap_config() executes krealloc(). If the bounds check fails and
cleanup frees the local ap_config pointer, does
mig_data->resuming_mig_state.ap_config still hold the freed pointer, leading
to a Double Free when vfio_ap_release_resuming_file() is called?

[Severity: Critical]
Is allocate_ap_config() allocating an undersized buffer if the input payload
is smaller than expected?

drivers/s390/crypto/vfio_ap_migration.c:allocate_ap_config() {
	...
	} else {
		config_size = len;
	}

	*ap_config = kzalloc(config_size, GFP_KERNEL_ACCOUNT);
	...
}

If userspace writes a payload strictly smaller than sizeof(struct
vfio_ap_config), and vfio_ap_resuming_write() considers the configuration
fully written, the undersized buffer is passed to do_post_copy_validation().

In queues_available_on_target_system():

	for (int i = 0; i < source_config->num_queues; i++) {
		apqn = source_config->qinfo[i].apqn;

Does this allow an out-of-bounds heap read based on the untrusted
and undersized config buffer?

[Severity: Critical]
Could the stack variable guest_matrix be used without initialization?

drivers/s390/crypto/vfio_ap_migration.c:vfio_ap_resuming_write() {
	...
	struct ap_matrix guest_matrix;
	...
		setup_ap_matrix_from_ap_config(mdev_name, ap_config,
					       &guest_matrix);
	...
}

In setup_ap_matrix_from_ap_config():

		if (!test_bit_inv(apid, guest_matrix->apm))
			set_bit_inv(apid, guest_matrix->apm);

It looks like guest_matrix is declared on the stack without being
zero-initialized, and setup_ap_matrix_from_ap_config() only sets individual
bits. Does this leave uninitialized bits in the apm, aqm, and adm bitmaps,
potentially granting the target guest hardware access to unauthorized AP
adapters and domains?

[Severity: High]
Are we missing the initialization of the config_sz field during the
STOP_COPY phase?

drivers/s390/crypto/vfio_ap_migration.c:vfio_ap_get_config() {
	ap_configuration = kzalloc(*ap_config_size, GFP_KERNEL_ACCOUNT);
	if (!ap_configuration)
		return -ENOMEM;

	ap_configuration->num_queues = num_queues;
	...
}

The allocation succeeds, but it doesn't look like
ap_configuration->config_sz is initialized before returning it to be copied
to user space. If this structure is transferred and the target host receives
config_sz == 0, would a fragmented write on the target erroneously call
krealloc() to shrink the buffer to merely len bytes, breaking fragmented
migrations?

[Severity: High]
Is there a locking issue in vfio_ap_release_mig_file() that could lead to a
data race and Use-After-Free?

drivers/s390/crypto/vfio_ap_migration.c:vfio_ap_release_mig_file() {
	...
	if (!matrix_mdev || !matrix_mdev->mig_data)
		return -ENODEV;

	if (filp == matrix_mdev->mig_data->stop_copy_mig_file)
		vfio_ap_release_stop_copy_file(matrix_mdev->mig_data);
	...
}

This accesses matrix_mdev->mig_data entirely locklessly. Since
vfio_ap_release_migration_data() and vfio_ap_transition_to_state() modify
and free this data under matrix_dev->mdevs_lock, could a concurrent close of
the migration file descriptor result in a Time-of-Check to Time-of-Use race?

If vfio_ap_release_mig_file() evaluates matrix_mdev->mig_data as valid but a
concurrent teardown thread kfrees it, would this result in a Use-After-Free
when dereferencing mig_data->stop_copy_mig_file?

[Severity: Medium]
Will vfio_ap_release_migration_data() leak ap_config when the device is
closed?

drivers/s390/crypto/vfio_ap_migration.c:vfio_ap_release_migration_data() {
	if (!matrix_mdev->mig_data)
		return;

	kfree(matrix_mdev->mig_data);
	matrix_mdev->mig_data = NULL;
}

This frees mig_data without cleaning up the dynamically allocated
mig_data->resuming_mig_state.ap_config buffer inside it.

Because the pointer is nulled, when VFS eventually drops the final reference
to the RESUMING migration file, vfio_ap_release_mig_file() will abort early
with -ENODEV. Will this bypass the kfree call within
vfio_ap_release_resuming_file() and permanently leak the allocated ap_config
memory?

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