Re: [PATCH v11 05/14] s390/vfio-ap: Callback to get/set vfio device mig state during guest migration

[sashiko-bot@kernel.org]

Thank you for your contribution! Sashiko AI review found 1 potential issue(s) to consider:
- [Medium] s390/vfio-ap: wrong SE guest check for migration in vfio_ap_set_state()
--

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

s390/vfio-ap: Callback to get/set vfio device mig state during guest migration

This commit implements the vfio_ap_set_state() and vfio_ap_get_state()
migration callbacks and defines vfio_ap_migration_ops for vfio-ap mediated
devices.

> diff --git a/drivers/s390/crypto/vfio_ap_migration.c b/drivers/s390/crypto/vfio_ap_migration.c
> index 6bcb910526c9e..8e4e04e039658 100644
> --- a/drivers/s390/crypto/vfio_ap_migration.c
> +++ b/drivers/s390/crypto/vfio_ap_migration.c
> @@ -143,6 +143,150 @@ static void vfio_ap_release_resuming_file(struct vfio_ap_migration_data *mig_dat

[ ... ]

> +static struct file *vfio_ap_set_state(struct vfio_device *vdev,
> +				      enum vfio_device_mig_state  new_state)
> +{
> +	int ret;
> +	struct file *filp = NULL;
> +	struct ap_matrix_mdev *matrix_mdev;
> +	enum vfio_device_mig_state next_state;
> +	struct vfio_ap_migration_data *mig_data;
> +
> +	mutex_lock(&matrix_dev->mdevs_lock);
> +
> +	matrix_mdev = container_of(vdev, struct ap_matrix_mdev, vdev);
> +	mig_data = matrix_mdev->mig_data;
> +
> +	/*
> +	 * The mig_data pointer is set in the vfio_ap_init_migration_data
> +	 * function which is called when the vfio-ap device fd is opened.
> +	 * Since the implicit pre-open state is RUNNING, a request to set
> +	 * RUNNING is a no-op. Any other state transition is invalid before
> +	 * open_device.
> +	 */
> +	if (!mig_data) {
> +		mutex_unlock(&matrix_dev->mdevs_lock);
> +		if (new_state == VFIO_DEVICE_STATE_RUNNING)
> +			return NULL;
> +		return ERR_PTR(-ENODEV);
> +	}

[Severity: Medium]
Does vfio_ap_set_state() allow migration state transitions for Protected
Virtualization (Secure Execution) KVM guests?

In vfio_ap_init_migration_data(), the check intended to skip initializing
migration data for SE guests calls ap_is_se_guest():

drivers/s390/crypto/vfio_ap_migration.c:vfio_ap_init_migration_data() {
	/*
	 * Live guest migration is not supported for SE guests, so there is no
	 * need to initialize the migration data.
	 */
	if (ap_is_se_guest())
		return 0;
}

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

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

On a KVM host, is_prot_virt_guest() is false, so ap_is_se_guest() always
evaluates to false and matrix_mdev->mig_data is unconditionally allocated
when vfio_ap_mdev_open_device() calls vfio_ap_init_migration_data().

In addition, a KVM guest only enters Protected Virtualization mode later
during guest boot via kvm_s390_pv_init_vm(), after open_device and
vfio_ap_mdev_set_kvm() have already run. Because vfio_ap_set_state() only
checks !mig_data and does not check whether matrix_mdev->kvm is a protected
guest (for example via kvm_s390_pv_is_protected(matrix_mdev->kvm)), wouldn't
migration state transitions be permitted on SE guests?

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