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

[sashiko-bot@kernel.org]

Thank you for your contribution! Sashiko AI review found 1 potential issue(s) to consider:
- [Medium] s390/vfio-ap: stale resuming_mig_data_complete after write past expected_sz
--

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

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

This commit implements the VFIO migration state transitions between STOP
and RESUMING, STOP_COPY, and RUNNING in vfio_ap_transition_to_state(),
cleaning up migration file state as needed.

> diff --git a/drivers/s390/crypto/vfio_ap_migration.c b/drivers/s390/crypto/vfio_ap_migration.c
> index 469fa09ec9087..cf38407d1b157 100644
> --- a/drivers/s390/crypto/vfio_ap_migration.c
> +++ b/drivers/s390/crypto/vfio_ap_migration.c

[ ... ]

> @@ -1052,7 +1048,7 @@ static ssize_t vfio_ap_resuming_write(struct file *filp, const char __user *buf,
>  		}
>  
>  		if (*pos == (loff_t)expected_sz) {
> -			ret = do_post_copy_processing(matrix_mdev, ap_config);
> +			ret = do_post_copy_validation(matrix_mdev, ap_config);
>  			matrix_mdev->mig_data->resuming_mig_data_complete = !ret;
>  		}
>  	}
> @@ -1146,20 +1142,46 @@ vfio_ap_transition_to_state(struct ap_matrix_mdev *matrix_mdev,
>  		return filp;
>  	}
>  
> -	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);
> +	/*
> +	 * Terminates the data transfer session of the vfio-ap device state
> +	 * to the destination guest.
> +	 */
> +	if (cur_state == VFIO_DEVICE_STATE_RESUMING &&
> +	    new_state == VFIO_DEVICE_STATE_STOP) {
> +		if (!mig_data->resuming_mig_data_complete)
> +			return ERR_PTR(-EINVAL);
> +
> +		/* Set resuming_mig_data_complete to false for clean reuse */
> +		matrix_mdev->mig_data->resuming_mig_data_complete = false;
> +		vfio_ap_release_resuming_file(matrix_mdev->mig_data);
> +		return NULL;
>  	}

[Severity: Medium]
Can the transition from VFIO_DEVICE_STATE_RESUMING to
VFIO_DEVICE_STATE_STOP in vfio_ap_transition_to_state() still succeed
after a subsequent write() to the migration file descriptor pushes
filp->f_pos past expected_sz and returns -EINVAL?

In vfio_ap_resuming_write(), once a valid vfio_ap_config of size
expected_sz (with num_queues < MAX_AP_QUEUES) has been written,
resuming_mig_data_complete is set to true:

drivers/s390/crypto/vfio_ap_migration.c:vfio_ap_resuming_write() {
	...
	memcpy((char *)ap_config + *pos, kbuf, len);
	*pos += len;

	if (*pos >= offsetofend(struct vfio_ap_config, num_queues)) {
		...
		if (*pos > (loff_t)expected_sz) {
			ret = -EINVAL;
			goto out_unlock;
		}

		if (*pos == (loff_t)expected_sz) {
			ret = do_post_copy_validation(matrix_mdev, ap_config);
			matrix_mdev->mig_data->resuming_mig_data_complete = !ret;
		}
	}
}

If userspace performs an additional write() on the same migration FD where
filp->f_pos + len <= VFIO_AP_CONFIG_MAX_SIZE, validate_resuming_write_parms()
succeeds, the extra bytes are copied into ap_config, and *pos advances
past expected_sz. The check for *pos > (loff_t)expected_sz then returns
-EINVAL, but resuming_mig_data_complete remains true.

When userspace subsequently transitions from VFIO_DEVICE_STATE_RESUMING to
VFIO_DEVICE_STATE_STOP, vfio_ap_transition_to_state() only checks
!mig_data->resuming_mig_data_complete and succeeds despite the trailing
data and failed write(). Should vfio_ap_resuming_write() clear
resuming_mig_data_complete (or avoid mutating ap_config and *pos) when a
write exceeds expected_sz?

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