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

[sashiko-bot@kernel.org]

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

New issues:
- [High] Migration stream file descriptors are not invalidated upon transitioning out of data transfer states, allowing untrusted userspace to modify running guest state.
- [High] The driver unconditionally allows the transition from RESUMING to STOP, failing to reject incomplete migration data as required by the VFIO specification.

Pre-existing issues:
- [Critical] Chunked or partial writes to the RESUMING fd leak memory and cause a NULL pointer dereference on subsequent chunks.
- [Critical] Migration file descriptors do not hold a reference to the `vfio_device` or `matrix_mdev`, leading to a Use-After-Free if the device is removed while the migration FD is kept open.
- [Critical] vfio_ap_release_mig_file accesses and modifies migration state without holding matrix_dev->mdevs_lock, leading to UAF.
- [High] Writing fewer than `sizeof(struct vfio_ap_config)` bytes to the resuming stream bypasses size validation, triggering a Heap Out-of-Bounds read.
- [High] The dynamically allocated `ap_config` is permanently leaked if the VFIO device is closed while the migration file descriptor remains open.
--

commit 1ef55b17508b1d713795918bce912d07492d77e0
Author: Anthony Krowiak <akrowiak@linux.ibm.com>

s390/vfio-ap: Transition device migration state to STOP

This commit updates the vfio-ap migration state machine to allow
transitions to the stop state. It allows returning null when
transitioning from resuming or stop copy to stop.

> diff --git a/drivers/s390/crypto/vfio_ap_migration.c b/drivers/s390/crypto/vfio_ap_migration.c
> index 9cd1e7a3f7430..1b526b743ad02 100644
> --- a/drivers/s390/crypto/vfio_ap_migration.c
> +++ b/drivers/s390/crypto/vfio_ap_migration.c
> @@ -1159,12 +1159,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 transitioning to the stop state here leave the active file stream
pointers open for abuse? By simply returning null without clearing
mig_data->resuming_mig_state.filp, userspace might be able to
transition the device to stop and then running, while keeping the
resuming file descriptor open to write to it.

In validate_resuming_write_parms(), it only validates that filp matches
the cached pointer without checking if the device is actually in the
VFIO_DEVICE_STATE_RESUMING state:

validate_resuming_write_parms() {
    if (filp != matrix_mdev->mig_data->resuming_mig_state.filp) {
        ret = -ENXIO;
        goto done;
    }
}

Could this trigger asynchronous mutation of the AP hardware
configuration of a live guest?

[Severity: High]
Does unconditionally returning null here bypass verifying the completeness
of the migration data transfer?

If userspace writes a partial configuration and transitions to stop,
will the device silently remain unconfigured without signaling an error
to userspace?

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

[Severity: Critical]
This is a pre-existing issue, but do chunked or partial writes to the
resuming file descriptor leak memory and cause a null pointer dereference?

If a user writes a partial chunk in vfio_ap_resuming_write() where the size
is less than cfg_sz, it skips assigning the dynamically allocated ap_config
back to the global state:

vfio_ap_resuming_write() {
    ...
    if (*pos + len == cfg_sz) {
        ret = do_post_copy_validation(mdev_name, ap_config);
    ...
}

On the subsequent write chunk, retrieving ap_config from mig_data fetches
null, which then gets passed to reallocate_ap_config():

reallocate_ap_config() {
    struct vfio_ap_config *ap_config = *mig_ap_config;
    ...
    cur_cfg_sz = ap_config->config_sz;
}

Will this dereference cause a kernel oops?

[Severity: High]
This is a pre-existing issue, but does writing fewer than the expected size
bypass size validation and trigger a heap out-of-bounds read?

In allocate_ap_config(), writing an undersized buffer sets config_size
to the short length:

allocate_ap_config() {
    ...
    } else {
        config_size = len;
    }
}

If vfio_ap_resuming_write() sees that the received size matches cfg_sz,
it assumes the configuration is fully received and passes the undersized
buffer for validation.

Since userspace controls num_queues, will iterating over it read out
of bounds?

queues_available_on_target_system() {
    for (int i = 0; i < source_config->num_queues; i++) {
        apqn = source_config->qinfo[i].apqn;
    ...
}

[Severity: High]
This is a pre-existing issue, but is the dynamically allocated ap_config
permanently leaked if the device is closed while the migration file
descriptor remains open?

If userspace closes the main device file descriptor, the container struct
is freed:

vfio_ap_release_migration_data() {
    ...
    kfree(matrix_mdev->mig_data);
    matrix_mdev->mig_data = NULL;
}

When the migration file descriptor is later closed, it bails out early
if the container is null, leaving the nested ap_config allocation orphaned:

vfio_ap_release_mig_file() {
    if (!matrix_mdev || !matrix_mdev->mig_data)
        return -ENODEV;
    ...
}

[Severity: Critical]
This is a pre-existing issue, but do migration file descriptors fail to
hold a reference to the vfio_device or matrix_mdev?

When opening the stream, anon_inode_getfile() is called without bumping
the reference count:

vfio_ap_open_file_stream() {
    filp = anon_inode_getfile("vfio_ap_mig_file", fops, matrix_mdev, flags);
    ...
}

If the device is removed via sysfs while the migration file descriptor
is open, will the final reference be dropped?

vfio_ap_mdev_remove() {
    ...
    vfio_put_device(&matrix_mdev->vdev);
}

Could subsequent operations on the open migration file descriptor
dereference the freed matrix_mdev and result in a use-after-free?

[Severity: Critical]
This is a pre-existing issue, but does accessing the migration state
locklessly in vfio_ap_release_mig_file() lead to a use-after-free?

The function checks mig_data without holding a lock:

vfio_ap_release_mig_file() {
    if (!matrix_mdev || !matrix_mdev->mig_data)
        return -ENODEV;

    if (filp == matrix_mdev->mig_data->stop_copy_mig_file)
    ...
}

If userspace concurrently triggers closure of the primary device, can
the memory be freed under the mdevs_lock?

vfio_ap_release_migration_data() {
    kfree(matrix_mdev->mig_data);
    matrix_mdev->mig_data = NULL;
}

Could this allow vfio_ap_release_mig_file() to dereference mig_data
immediately after it is freed?

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