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

Thank you for your contribution! Sashiko AI review found 4 potential issue(s) to consider:

New issues:
- [High] Memory leak and silent transmission of stale device state due to missing buffer cleanup on FSM STOP transition.
- [High] Migration data file descriptors allow out-of-band manipulation of the active AP configuration.

Pre-existing issues:
- [Critical] Unprotected concurrent ioctls cause Use-After-Free, Double Free, and Memory Leaks during migration reads/writes.
- [High] RESUMING state machine is permanently broken by an incomplete reset.
--

--- Patch [11]: [PATCH v7 11/15] s390/vfio-ap: Transition device migration state to STOP ---
commit e7e2429c70da0ea0a7016c096567e78834fcc351
Author: Anthony Krowiak <[email protected]>

    s390/vfio-ap: Transition device migration state to STOP
    
    This commit updates the VFIO AP migration state machine to handle
    transitions to the STOP state from RESUMING or STOP_COPY. It returns
    NULL instead of an error, stating that the VFIO migration framework
    will clean up file streams and associated data when releasing files.

> There  only thing to do at this point is return
> NULL to the caller since the file streams and associated data will get
> cleaned up as a result of the VFIO migration framework releasing the files.

[Severity: High]
Is this assumption about the file streams and associated data getting cleaned
up accurate?

Looking at the file release callback:

vfio_ap_release_mig_file() {
    ...
    vfio_device_put_registration(&matrix_mdev->vdev);
    return 0;
}

The callback only releases the VFIO registration reference and does not free
the state buffers like stop_copy_mig_file.ap_config or
resuming_mig_file.ap_config.

Could this lead to a memory leak and the silent transmission of stale device
state to the destination host on subsequent migration attempts, for instance
if we skip allocating and fetching a fresh configuration in
vfio_ap_stop_copy_read() because ap_config was leaked and left populated?

> diff --git a/drivers/s390/crypto/vfio_ap_migration.c b/drivers/s390/crypto/vfio_ap_migration.c
> index 4dd7373c3d9d9..30160e3dddedd 100644
> --- a/drivers/s390/crypto/vfio_ap_migration.c
> +++ b/drivers/s390/crypto/vfio_ap_migration.c
> @@ -1511,12 +1511,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 successfully change the FSM state without clearing
the active filp pointers in mig_data?

If userspace keeps the migration file descriptor open (or duplicates it)
during an FSM transition to RUNNING, and then issues a write on the stale
file descriptor, could this allow out-of-band manipulation of the KVM guest's
AP configuration while the guest is actively running?

The file operations like validate_resuming_write_parms() appear to only check
if the incoming filp pointer matches the stored pointer:

validate_resuming_write_parms() {
    ...
    if (filp != resuming_mig_file.filp)
        return -ENXIO;
    ...
}

This happens without ensuring the FSM is actually in the RESUMING state,
potentially permitting writes during RUNNING.

[Severity: High]
This is a pre-existing issue, but does a reset or an aborted RESUMING phase
leave the RESUMING state machine permanently broken?

When vfio_ap_release_resuming_file() is called, it frees
resuming_mig_file.ap_config but fails to clear resuming_config_buf.filled:

vfio_ap_release_resuming_file() {
    kvfree(mig_data->resuming_mig_file.ap_config);
    mig_data->resuming_mig_file.ap_config = NULL;
    mig_data->resuming_mig_file.config_sz = 0;
    mig_data->resuming_mig_file.filp = NULL;
}

If userspace initiates a new RESUMING phase, validate_resuming_write_parms()
will evaluate this condition as true:

    if ((resuming_config_buf.filled && !resuming_mig_file.ap_config) ||
        (!resuming_config_buf.filled && resuming_mig_file.ap_config))
        return -EFAULT;

Would this unconditionally reject all subsequent writes with -EFAULT?

[Severity: Critical]
This is a pre-existing issue, but are the migration reads and writes protected
against concurrent ioctls?

For example, in vfio_ap_resuming_write(), mdevs_lock is dropped before
copying data from userspace and interacting with set_new_ap_configuration():

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

    if (copy_from_user((char *)ap_config + write_pos, buf, write_len)) {
    ...
    ret = set_new_ap_configuration(matrix_mdev, ap_config, cfg_sz);
    ...
    if (!new_allocation)
        kvfree(ap_config);
    ...
}

If a concurrent VFIO_DEVICE_RESET ioctl occurs while the lock is dropped,
could set_new_ap_configuration() assign a locally allocated ap_config into
the now-NULL resuming_mig_file.ap_config?

Would the outer function then call kvfree(ap_config), leaving a dangling
pointer in the device state that could subsequently be used and double-freed?

Similarly, in vfio_ap_stop_copy_read(), the lock is dropped before
copy_to_user():

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

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

Could a concurrent reset free mig_file->ap_config before or during the copy,
leading to an information leak of freed kernel memory to userspace?

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

-- 
Sashiko AI review · https://sashiko.dev/#/patchset/[email protected]?part=11