Re: [PATCH v5 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 6 potential issue(s) to consider:
- [Critical] TOCTOU data race on filp->f_pos in migration stream read/write operations causing heap out-of-bounds access.
- [High] Transitioning from RUNNING to STOP state fails to quiesce AP hardware queues, violating the VFIO migration protocol.
- [High] Unbalanced fput() in vfio_ap_release_mig_files() causes a Use-After-Free of the migration struct file.
- [High] Use-After-Free of `matrix_mdev` via migration data file descriptor.
- [Medium] Stale migration data due to failure to clear ap_config across repeated STOP_COPY or RESUMING state transitions.
- [Low] Missing space in the newly added comment.
--

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

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

This commit handles the transition between STOP and RUNNING migration states
by setting the new state without returning an error. It assumes the device is
operating normally and that no internal state needs to be incorporated on the
target since vfio-ap does not virtualize a DMA device.

> diff --git a/drivers/s390/crypto/vfio_ap_migration.c b/drivers/s390/crypto/vfio_ap_migration.c
> index 21cd9810a18c4..8876338d28980 100644
> --- a/drivers/s390/crypto/vfio_ap_migration.c
> +++ b/drivers/s390/crypto/vfio_ap_migration.c
> @@ -1317,12 +1317,17 @@ vfio_ap_transition_to_state(struct ap_matrix_mdev *matrix_mdev,
>  		return NULL;
>  	}
>  
> +	/*
> +	 * These states indicate migration has either not been initiated or
> +	 * has completed and the vfio-ap device is operating normally.Since the

[Severity: Low]
This isn't a bug, but there is a missing space after "normally." in the comment.

> +	 * 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 this return from the RUNNING to STOP transition arc leave the physical
AP hardware queues active?

The VFIO migration specification mandates that a device must be fully quiesced
in the STOP state, meaning no DMA, no asynchronous execution, and no interrupts.

Since this arc simply returns NULL instead of quiescing the device, could
pending asynchronous hardware operations continue and write to guest memory
after QEMU considers the device stopped?

While reviewing this patch, I noticed a few related regressions in the
migration file handling.

[Severity: High]
Can an unbalanced fput() in vfio_ap_release_mig_files() cause a use-after-free
of the migration file?

When anon_inode_getfile() creates the migration stream, it returns a file with
a refcount of 1. If userspace triggers a reset while the migration file stream
is open, the driver executes this fput() without having taken its own reference
via get_file():

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;
        }
        ...
}

Does this drop the userspace-owned reference to 0, freeing the file while the
user's file descriptor is still open?

[Severity: Medium]
Could this logic result in migrating stale device state?

If userspace transitions the device STOP_COPY -> STOP -> STOP_COPY while
keeping the old migration stream file descriptor open, ap_config is not cleared
because the file release callback hasn't fired:

drivers/s390/crypto/vfio_ap_migration.c:vfio_ap_stop_copy_read() {
        ...
        matrix_mdev = filp->private_data;
        if (!matrix_mdev->mig_data->stop_copy_mig_file.ap_config) {
                ret = vfio_ap_get_config(matrix_mdev);
        ...
}

Will the next STOP_COPY transition skip fetching the updated hardware state and
silently serve the stale configuration?

[Severity: High]
Is there a possibility of a use-after-free of matrix_mdev via the migration
file descriptor?

The migration file descriptor is created without taking a reference to the
vfio_device:

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 userspace keeps the migration file descriptor open but closes the VFIO
device file descriptor, matrix_mdev and mig_data are freed. A subsequent read
or write on the migration file descriptor would dereference them:

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

Does the driver need to tie the lifetime of the migration file descriptor to
the vfio_device?

[Severity: Critical]
Does this code allow concurrent modifications to cause an out-of-bounds access
on the kernel heap?

Because the migration file is opened with stream_open(), the VFS sets
FMODE_STREAM and bypasses f_pos_lock, allowing concurrent I/O syscalls.

In vfio_ap_stop_copy_read(), pos is assigned to &filp->f_pos and validated
under mdevs_lock. The lock is then dropped before dereferencing pos during
the copy:

drivers/s390/crypto/vfio_ap_migration.c:vfio_ap_stop_copy_read() {
        ...
        pos = &filp->f_pos;
        ret = validate_stop_copy_read_parms(filp, pos, len);
        ...
        mutex_unlock(&matrix_dev->mdevs_lock);

        if (copy_to_user(buf, (char *)ap_config + *pos, len)) {
        ...
}

If another thread advances filp->f_pos after the bounds check but before the
copy, could copy_to_user() read past the end of the ap_config buffer?

A similar issue appears during the write operation:

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

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

Could this lead to copy_from_user() writing past the buffer if filp->f_pos is
advanced concurrently?

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