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

On 7/27/26 2:28 PM, [email protected] wrote:
> 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 <[email protected]>
>
> 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.

The NULL return for STOP<->RUNNING transitions is the correct
VFIO API response when no data fd needs to be handed to
userspace — it does not touch any filp field in mig_data.
The scenario you describe — a new STOP->STOP_COPY transition
silently overwriting a still-live stop_copy_mig_file.filp —
cannot happen, for the following reasons:

File descriptor cleanup is always performed before a new fd
can be opened. vfio_ap_release_mig_files() is called by
both vfio_ap_reset_migration_state() (on VFIO_DEVICE_RESET)
and vfio_ap_release_migration_data() (on device close),
both under mdevs_lock. Either path nulls out
stop_copy_mig_file.filp before any subsequent
STOP->STOP_COPY transition can reach the code that writes
to it.

vfio_device_put_registration() is always balanced.
vfio_ap_release_mig_file() — the file_operations.release
handler invoked when userspace closes the fd —
unconditionally calls vfio_device_put_registration()
regardless of mig_data state. There is no code path that
causes it to be skipped.

The stale-data scenario does not apply.
validate_stop_copy_read_parms() rejects any read where
mig_data->stop_copy_mig_file.filp != filp. A fd from a
previous session will fail that check with -EINVAL and
never serve any data.

>
> [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.

This is a valid observation. At the commit under review,
vfio_ap_resuming_write() drops mdevs_lock for
copy_from_user() while holding a raw pointer to an
ap_config buffer that has not yet been stored into mig_data.
For subsequent writes (non-new-allocation path), the pointer
is retrieved from mig_data->resuming_mig_file.ap_config
under the lock but then used after the lock is dropped. A
concurrent VFIO_DEVICE_RESET calling
vfio_ap_release_mig_files() under the same lock could
kvfree() that buffer while the write path is copying into
it.

This will be addressed in the next patch series.

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