Re: [PATCH v2 10/16] s390/vfio-ap: Transition device migration state from RESUMING to STOP

Thank you for your contribution! Sashiko AI review found 2 potential issue(s) to consider:
- [Critical] Userspace can trigger a host kernel panic via a NULL pointer dereference by calling read/write on the migration file descriptor concurrently with or after a state transition.
- [Critical] NULL pointer dereference during post-copy validation if a target queue is not bound to the vfio_ap driver.
--

--- Patch [10]: [PATCH v2 10/16] s390/vfio-ap: Transition device migration state from RESUMING to STOP ---
commit 198a0563141b8b9148b57d67d870dea7883422b2
Author: Anthony Krowiak <[email protected]>

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

Leaving the RESUMING state terminates a data transfer of the internal
state of the vfio-ap device on the source host to the vfio-ap device on the
target host. This state transition indicates the vfio_ap driver should
complete the incorporation of data written to the data transfer FD. Since
a vfio-ap device does not virtualize a physical device, the only thing
left to do is release the migration files used to help facilitate the
migration process.

> diff --git a/drivers/s390/crypto/vfio_ap_migration.c b/drivers/s390/crypto/vfio_ap_migration.c
> index ce16028e50a41..ebb3b78b1e195 100644
> --- a/drivers/s390/crypto/vfio_ap_migration.c
> +++ b/drivers/s390/crypto/vfio_ap_migration.c
> @@ -915,10 +915,20 @@ vfio_ap_transition_to_state(struct ap_matrix_mdev *matrix_mdev,
>  		return migf->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; so, the only
> +	 * thing left to do is release the migration files used to process
> +	 * the vfio device migration. Note that this state transition is for
> +	 * the vfio-ap device on the target host.
> +	 */
>  	if (cur_state == VFIO_DEVICE_STATE_RESUMING &&
>  	    new_state == VFIO_DEVICE_STATE_STOP) {
> -		/* TODO */
> -		return ERR_PTR(-EOPNOTSUPP);
> +		vfio_ap_release_mig_files(matrix_mdev);

[Severity: Critical]
Can this regression cause a host kernel panic via a NULL pointer
dereference if userspace calls read or write on the migration file
descriptor concurrently with or after this state transition?

When vfio_ap_release_mig_files() is called, it sets migf->matrix_mdev = NULL
and migf->disabled = true.

If a userspace process continues to read or write to the migration file
descriptor, it can block on matrix_dev->mdevs_lock and then blindly
dereference migf->matrix_mdev once unblocked:

drivers/s390/crypto/vfio_ap_migration.c:vfio_ap_resume_write() {
    ...
    mig_data = migf->matrix_mdev->mig_data;
    ret = validate_resume_write_parms(migf, len, pos);
    ...
}

Does the error logging also directly dereference migf->matrix_mdev after
confirming migf->disabled is true, even though disabled == true guarantees
matrix_mdev == NULL?

drivers/s390/crypto/vfio_ap_migration.c:validate_save_read_parms() {
    ...
    if (migf->disabled) {
        dev_err(migf->matrix_mdev->vdev.dev,
                "%s (%d): migration file is disabled\n",
                __func__, __LINE__);
    ...
}

[Severity: Critical]
Is it also possible to hit a NULL pointer dereference during post-copy
validation if a target queue is not bound to the vfio_ap driver?

If userspace writes an AP configuration containing an apqn that is assigned to
the guest matrix, but the physical queue object is not bound to the host
vfio_ap driver (e.g., it is offline or unassigned), vfio_ap_mdev_get_queue()
returns NULL.

drivers/s390/crypto/vfio_ap_migration.c:matrixes_compatible() {
    ...
    q = vfio_ap_mdev_get_queue(matrix_mdev, apqn);
    memcpy(&src_hwinfo, &migf->ap_config->qinfo[i].data,
           sizeof(src_hwinfo));

    if (!qinfo_compatible(matrix_mdev, apqn, &src_hwinfo, &q->hwinfo))
    ...
}

Will this dereference q->hwinfo and pass it to qinfo_compatible() when q is
NULL, leading to a host crash?

> +
> +		return NULL;
>  	}

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