Re: [PATCH v5 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:

New issues:
- [High] Transitioning out of data transfer states (STOP_COPY -> STOP or RESUMING -> STOP) returns NULL without cleaning up the active migration file state, causing stale data reuse and memory leaks.

Pre-existing issues:
- [Critical] Migration file descriptors lack a reference count to `matrix_mdev`, allowing a Use-After-Free if the device is unbound while the file is open.
- [Critical] Calling `fput()` on a migration file stream without holding an additional reference causes a structural Use-After-Free of the file object.
- [Critical] Writing fewer than `sizeof(struct vfio_ap_config)` bytes to the resuming FD bypasses validation and causes an Out-Of-Bounds Read kernel panic.
--

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

This commit modifies the vfio-ap driver to support transitioning to the STOP
state during migration. It updates the state machine to return NULL instead
of -EOPNOTSUPP when transitioning out of the STOP_COPY or RESUMING states.

> diff --git a/drivers/s390/crypto/vfio_ap_migration.c b/drivers/s390/crypto/vfio_ap_migration.c
> index 7a9a8f63a76d3..21cd9810a18c4 100644
> --- a/drivers/s390/crypto/vfio_ap_migration.c
> +++ b/drivers/s390/crypto/vfio_ap_migration.c
> @@ -1304,12 +1304,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 calling vfio_ap_release_mig_files() leak the
active migration file state?

If the migration is aborted and a new one starts before userspace closes the
old file descriptor, the new session overwrites the filp pointer but does not
free ap_config. Will the new read operation then see a non-NULL ap_config and
silently reuse the stale configuration snapshot from the aborted migration?

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

[Severity: Critical]
This is a pre-existing issue, but does creating the migration file descriptor
without incrementing the VFIO device's reference count allow a use-after-free?

drivers/s390/crypto/vfio_ap_migration.c:vfio_ap_open_file_stream() {
    ...
	filp = anon_inode_getfile("vfio_ap_mig_file", fops, matrix_mdev, flags);
    ...
}

If a user holds the migration file descriptor open and unbinds the device via
sysfs, vfio_unregister_group_dev() can complete because the migration file
descriptor holds no reference to the device. Any future read, write, or close
on the file descriptor will dereference the freed matrix_mdev:

drivers/s390/crypto/vfio_ap_migration.c:vfio_ap_release_mig_file() {
    ...
	matrix_mdev = filp->private_data;
	if (!matrix_mdev || !matrix_mdev->mig_data) {
    ...
}

[Severity: Critical]
This is a pre-existing issue, but does calling fput() here cause a structural
use-after-free of the file object?

drivers/s390/crypto/vfio_ap_migration.c:vfio_ap_release_mig_files() {
    ...
	if (mig_data->stop_copy_mig_file.filp) {
		fput(mig_data->stop_copy_mig_file.filp);
		mig_data->stop_copy_mig_file.filp = NULL;
	}
    ...
}

Since vfio_ap_open_file_stream() allocates the file using anon_inode_getfile()
with a refcount of 1, and this single reference is consumed by the VFIO core
when it installs the file into the userspace file descriptor table, the driver
does not own a reference to the file.

If the device state is reset and fput() is called, it artificially drops the
refcount to 0, freeing the struct file while userspace still holds the file
descriptor open.

[Severity: Critical]
This is a pre-existing issue, but can writing fewer than
sizeof(struct vfio_ap_config) bytes to the resuming file descriptor bypass
validation and cause an out-of-bounds read?

If userspace writes a small payload (e.g., 4 bytes), allocate_ap_config()
allocates those 4 bytes directly:

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

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

This allows the write handler's completion check to instantly pass:

drivers/s390/crypto/vfio_ap_migration.c:vfio_ap_resuming_write() {
    ...
	/* Check if we've completed writing the entire configuration */
	if (*pos + len == cfg_sz) {
		/* Acquire locks required to update the guest's AP config */
		...
		ret = do_post_copy_validation(mdev_name, ap_config);
    ...
}

The validation will then read a user-controlled num_queues value from the
undersized buffer and loop over the qinfo array, reading heap memory
drastically out of bounds:

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

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