Re: [PATCH v10 13/15] s390/vfio-ap: Set up for vfio-ap device migration

[sashiko-bot@kernel.org]

Thank you for your contribution! Sashiko AI review found 2 potential issue(s) to consider:
- [High] s390/vfio-ap: wrong SE guest check via ap_is_se_guest() on KVM host
- [Medium] s390/vfio-ap: stale completion flag and f_pos advance in vfio_ap_resuming_write()
--

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

s390/vfio-ap: Set up for vfio-ap device migration

This commit wires up VFIO migration operations and sets the
VFIO_MIGRATION_STOP_COPY flag on the vfio-ap mdev during device probe to
enable live migration support.

> diff --git a/drivers/s390/crypto/vfio_ap_migration.c b/drivers/s390/crypto/vfio_ap_migration.c
> index 38098968eaa2c..660b5a21fe9e8 100644
> --- a/drivers/s390/crypto/vfio_ap_migration.c
> +++ b/drivers/s390/crypto/vfio_ap_migration.c
> @@ -212,6 +212,30 @@ static int vfio_ap_release_mig_file(struct inode *file_inode, struct file *filp)
>  	return 0;
>  }
>  
> +static const struct vfio_migration_ops vfio_ap_migration_ops;
> +
> +/**
> + * vfio_ap_setup_vfio_device_migration:
> + *
> + * Plug the object containing the function pointers to the migration callbacks
> + * required to facilitate VFIO AP migration into the mdev
> + *
> + *
> + * @matrix_mdev: pointer to object containing the mdev state
> + */
> +void vfio_ap_setup_vfio_device_migration(struct ap_matrix_mdev *matrix_mdev)
> +{
> +	/*
> +	 * Live guest migration is not supported for SE guests, so there is no
> +	 * need to initialize the migration capabilities.
> +	 */
> +	if (ap_is_se_guest())
> +		return;

[Severity: High]
Does calling ap_is_se_guest() here and in vfio_ap_init_migration_data()
actually detect a Secure Execution guest on the KVM host?

In drivers/s390/crypto/ap_bus.c, ap_is_se_guest() checks whether the
currently running kernel is itself a Protected Virtualization guest:

bool ap_is_se_guest(void)
{
	return is_prot_virt_guest() && ap_sb_available();
}

Since vfio_ap runs in the KVM host kernel where is_prot_virt_guest() is
always false, ap_is_se_guest() will always evaluate to false on the host.

Furthermore, at vfio_ap_mdev_probe() -> vfio_ap_setup_vfio_device_migration()
time, matrix_mdev->kvm is still NULL, and a guest only enters Protected
Virtualization mode at runtime via KVM_S390_PV_COMMAND. Should the SE guest
check instead be performed dynamically against matrix_mdev->kvm via
kvm_s390_pv_is_protected() when handling migration state transitions in
vfio_ap_set_state() or vfio_ap_transition_to_state()?

> +
> +	matrix_mdev->vdev.migration_flags = VFIO_MIGRATION_STOP_COPY;
> +	matrix_mdev->vdev.mig_ops = &vfio_ap_migration_ops;
> +}

[Severity: Medium]
Now that vfio_ap_migration_ops is wired up and exposes the migration stream
and state transitions to userspace, can vfio_ap_resuming_write() leave
resuming_mig_data_complete set to true or leave filp->f_pos advanced when a
write fails?

In 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)) {
		size_t expected_sz;

		if (ap_config->num_queues > MAX_AP_QUEUES) {
			ret = -EINVAL;
			goto out_unlock;
		}

		expected_sz = sizeof(struct vfio_ap_config) +
			      ap_config->num_queues *
			      sizeof(struct vfio_ap_queue_info);

		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 writes a valid payload where *pos == expected_sz,
resuming_mig_data_complete is set to true. If userspace then issues an extra
write to the resuming FD before transitioning to VFIO_DEVICE_STATE_STOP, the
extra bytes are copied into ap_config, *pos is incremented past expected_sz,
and the function returns -EINVAL without clearing resuming_mig_data_complete.
A subsequent transition from VFIO_DEVICE_STATE_RESUMING to
VFIO_DEVICE_STATE_STOP in vfio_ap_transition_to_state() only checks
mig_data->resuming_mig_data_complete and will still succeed.

Also, when any of the checks on num_queues, expected_sz, or
do_post_copy_validation() fail, *pos has already been incremented even though
vfio_ap_resuming_write() returns an error. Should the validation happen
before advancing *pos and clear resuming_mig_data_complete on error?

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