Re: [PATCH v4 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 7 potential issue(s) to consider:

New issues:
- [High] The transition to VFIO_DEVICE_STATE_STOP does not quiesce the AP hardware queues, leaving the device running during and after migration.

Pre-existing issues:
- [Critical] Writing fewer bytes than `sizeof(struct vfio_ap_config)` leads to a ZERO_SIZE_PTR panic or Slab Out-of-Bounds read.
- [Critical] A partial write of the AP configuration at `pos == 0` permanently leaks the allocated memory and leads to a NULL pointer dereference crash.
- [Critical] Error paths and reallocation logic in `vfio_ap_resuming_write` free `ap_config` without clearing the global pointer in `mig_data`.
- [High] The `guest_matrix` stack variable is used without initialization when setting up the AP matrix.
- [High] Concurrent `write` operations on the migration file descriptors cause a data race on `filp->f_pos`, bypassing bounds checks and corrupting the migration stream state.
- [High] `vfio_ap_release_mig_file` accesses and modifies `mig_data` internal pointers locklessly, racing with concurrent device state transitions.
--

--- Patch [12]: [PATCH v4 12/15] s390/vfio-ap: Transition device migration state from STOP to RUNNING and vice versa ---
commit 6d2ef5d46b7ff5343fd3f1a4ec81fec852894cf9
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 the STOP and RUNNING states for the
vfio-ap device during migration. It indicates that the device is operating
normally and sets the new state by returning NULL instead of an error.

> diff --git a/drivers/s390/crypto/vfio_ap_migration.c b/drivers/s390/crypto/vfio_ap_migration.c
> index 1b526b743ad02..572db04c4ac15 100644
> --- a/drivers/s390/crypto/vfio_ap_migration.c
> +++ b/drivers/s390/crypto/vfio_ap_migration.c
> @@ -1172,12 +1172,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
> +	 * 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 transition to VFIO_DEVICE_STATE_STOP fully quiesce the hardware?

The VFIO migration specification mandates that VFIO_DEVICE_STATE_STOP must
fully stop the device from executing or performing DMA. By just returning NULL
here without calling vfio_ap_mdev_reset_queues() or similar, won't the AP
queues continue to asynchronously process in-flight requests and perform DMA
writes to the guest's NIB physical address?

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

While reviewing this migration code, I also noticed several pre-existing issues
in the vfio_ap_migration.c implementation:

[Severity: High]
This is a pre-existing issue, but does setup_ap_matrix_from_ap_config() use an
uninitialized stack variable?

In vfio_ap_resuming_write(), guest_matrix is declared on the stack without
initialization:

    struct ap_matrix guest_matrix;
    ...
    setup_ap_matrix_from_ap_config(mdev_name, ap_config, &guest_matrix);

Inside setup_ap_matrix_from_ap_config(), it only sets bits to 1:

    if (!test_bit_inv(apid, guest_matrix->apm))
        set_bit_inv(apid, guest_matrix->apm);

Can this leave uninitialized stack garbage in the arrays (like the adm array),
causing the target guest to be configured with unintended AP adapter access?


[Severity: Critical]
This is a pre-existing issue, but does allocate_ap_config() handle small
lengths correctly?

If a user writes a small chunk where len < sizeof(struct vfio_ap_config), the
code falls back to allocating len bytes:

    if (len >= sizeof(struct vfio_ap_config)) {
        config_size = calculate_ap_config_size(tmp_ap_config.num_queues);
        ...
    } else {
        config_size = len;
    }

    *ap_config = kzalloc(config_size, GFP_KERNEL_ACCOUNT);

If a user writes 0 bytes, won't kzalloc(0) return ZERO_SIZE_PTR, causing a
kernel panic upon dereference?

Also, if len is small (e.g., 8 bytes), this bypasses the minimum size check.
Could this allow the tiny buffer to be passed to do_post_copy_validation(),
which reads num_queues from the heap, potentially leading to an out-of-bounds
memory access?


[Severity: Critical]
This is a pre-existing issue, but does a partial write leak memory and cause a
NULL pointer dereference?

In vfio_ap_resuming_write(), if the first write is partial, the newly
allocated pointer is not saved to the global state until the entire write
is complete:

    if (*pos + len == cfg_sz) {
        ...
        mig_data->resuming_mig_state.ap_config = ap_config;
    }

On the next write chunk, the code retrieves the pointer from mig_data:

    ap_config = mig_data->resuming_mig_state.ap_config;
    ret = reallocate_ap_config(&ap_config, len);

Since the pointer was never stored on the partial write, it retrieves a
NULL pointer. Inside reallocate_ap_config(), it immediately dereferences
ap_config to read config_sz. Won't this cause a guaranteed NULL pointer
dereference crash? Also, doesn't the initially allocated buffer leak entirely?


[Severity: Critical]
This is a pre-existing issue, but do error paths in vfio_ap_resuming_write()
leave dangling pointers?

If an error occurs on a subsequent chunk (such as *pos + len > cfg_sz), the
cleanup path frees the pointer:

    ap_config = mig_data->resuming_mig_state.ap_config;
    ...
cleanup:
    kfree(ap_config);

However, mig_data->resuming_mig_state.ap_config is not cleared to NULL. When
the file descriptor is subsequently closed, vfio_ap_release_mig_file() will
unconditionally call kfree() on it again. Can this trigger a double free?


[Severity: High]
This is a pre-existing issue, but is there a data race on the file position?

vfio_ap_resuming_write() uses the file's internal position and locklessly
updates it after dropping mdevs_lock:

    mutex_unlock(&matrix_dev->mdevs_lock);

    if (copy_from_user((char *)ap_config + *pos, buf, len)) {
        ...
    ret = len;
    *pos += len;

Since the file stream is opened with stream_open(), FMODE_ATOMIC_POS is
cleared, and the VFS layer does not serialize writes with f_pos_lock. Could
concurrent write syscalls race to read and update *pos, bypassing bounds
checks and corrupting the migration state?


[Severity: High]
This is a pre-existing issue, but does vfio_ap_release_mig_file() safely
handle concurrent device state transitions?

The VFS release callback checks filp and frees ap_config locklessly:

    else if (filp == matrix_mdev->mig_data->resuming_mig_state.filp)
        vfio_ap_release_resuming_file(matrix_mdev->mig_data);

If a concurrent VFIO ioctl transitions the device state and overwrites
mig_data->resuming_mig_state.filp before the old file descriptor is released,
won't this callback fail to match the old filp? Could this bypass cleanup and
leak memory, or improperly free memory belonging to a new migration session?

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