Re: [PATCH v6 12/15] s390/vfio-ap: Transition device migration state from STOP to RUNNING and vice versa

[sashiko-bot@kernel.org]

Thank you for your contribution! Sashiko AI review found 2 potential issue(s) to consider:
- [Critical] Concurrent device reset, closure, or state transitions during migration data transfer cause Use-After-Free or Write-After-Free of `ap_config`.
- [High] Overwriting the active migration file descriptor during state transitions allows silent migration of stale state and leaks file references.
--

--- Patch [12]: [PATCH v6 12/15] s390/vfio-ap: Transition device migration state from STOP to RUNNING and vice versa ---
commit 2b837152c9f64c5082507b5aa41ffc56027c1e29
Author: Anthony Krowiak <akrowiak@linux.ibm.com>

s390/vfio-ap: Transition device migration state from STOP to RUNNING and vice versa

The transition from STOP to RUNNING and vice versa indicates that
the vfio device is operating normally and migration is not
currently taking place. In this case, just set the new state.

> diff --git a/drivers/s390/crypto/vfio_ap_migration.c b/drivers/s390/crypto/vfio_ap_migration.c
> index fc4cb893f0f00..1c385908fc39b 100644
> --- a/drivers/s390/crypto/vfio_ap_migration.c
> +++ b/drivers/s390/crypto/vfio_ap_migration.c
> @@ -1280,12 +1280,17 @@ vfio_ap_transition_to_state(struct ap_matrix_mdev *matrix_mdev,
[ ... ]
> +	/*
> +	 * These states indicate migration has either not been initiated or
> +	 * has completed and the vfio-ap device is operating normally. 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_STOP &&
>  	     new_state == VFIO_DEVICE_STATE_RUNNING) ||
>  	    (cur_state == VFIO_DEVICE_STATE_RUNNING &&
>  	     new_state == VFIO_DEVICE_STATE_STOP)) {
> -		/* TODO */
> -		return ERR_PTR(-EOPNOTSUPP);
> +		return NULL;

[Severity: High]
Does returning NULL here instead of -EOPNOTSUPP allow userspace to silently
leak the active migration file and state?

If a live migration is aborted and transitioned gracefully from STOP to
RUNNING, the old file descriptor could remain active in mig_data because
the driver still holds a reference to the file.

If the process later starts a new migration and transitions to STOP_COPY
again, it looks like vfio_ap_transition_to_state() unconditionally
overwrites mig_data->stop_copy_mig_file.filp:

vfio_ap_transition_to_state() {
    ...
    if (cur_state == VFIO_DEVICE_STATE_STOP &&
        new_state == VFIO_DEVICE_STATE_STOP_COPY) {
        ...
        mig_data->stop_copy_mig_file.filp = filp;
        return filp;
    }
    ...
}

Will this cause the driver's reference to the old file to be permanently lost?

If the old fd is read, vfio_ap_stop_copy_read() might observe that
mig_file->ap_config is already populated and skip updating it, serving
stale configuration to the target.

Additionally, when userspace closes the old fd, its release handler might
fail to match the overwritten filp pointer, which would skip
vfio_device_put_registration() and permanently pin the device.

[Severity: Critical]
With these new state transitions, is there also a risk of use-after-free or
write-after-free regressions during concurrent migration data transfers?

If a userspace process calls read() or write() on the migration file
descriptor, mdevs_lock is temporarily dropped during the data copy:

vfio_ap_stop_copy_read() {
    ...
    mutex_unlock(&matrix_dev->mdevs_lock);

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

Similarly in vfio_ap_resuming_write():

vfio_ap_resuming_write() {
    ...
    mutex_unlock(&matrix_dev->mdevs_lock);

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

Can a concurrent thread issue a VFIO_DEVICE_RESET ioctl, close the
device fd, or trigger a state transition that acquires mdevs_lock and
frees ap_config and mig_data?

If so, when the read/write thread resumes execution, it appears it
would access the freed ap_config memory, leading to arbitrary kernel
memory corruption or information leaks.

>  	}
>  
>  	/* vfio_mig_get_next_state() does not use arcs other than the above */

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