Re: [PATCH v4 13/15] s390/vfio-ap: Callback to get the size of data to be migrated during guest migration

[sashiko-bot@kernel.org]

Thank you for your contribution! Sashiko AI review found 2 potential issue(s) to consider:
- [High] vfio_ap_resuming_write leaks memory and fails subsequent writes if userspace performs a partial write during migration resuming.
- [High] vfio_ap_get_config does not initialize the config_sz field of the payload, sending 0 to the migration target.
--

--- Patch [13]: [PATCH v4 13/15] s390/vfio-ap: Callback to get the size of data to be migrated during guest migration ---
commit cf31fd21666e32de812c2582ed7358837c47d2bd
Author: Anthony Krowiak <akrowiak@linux.ibm.com>

s390/vfio-ap: Callback to get the size of data to be migrated during guest migration

This commit adds a callback to get the estimated data length required
to complete reading of the vfio device's internal state. This is mandatory
for VFIO_DEVICE_FEATURE_MIGRATION support.

> diff --git a/drivers/s390/crypto/vfio_ap_migration.c b/drivers/s390/crypto/vfio_ap_migration.c
> index 572db04c4ac15..83e1ce2c40c0c 100644
> --- a/drivers/s390/crypto/vfio_ap_migration.c
> +++ b/drivers/s390/crypto/vfio_ap_migration.c
> @@ -1283,7 +1283,18 @@ static int vfio_ap_get_state(struct vfio_device *vdev,
>  static int vfio_ap_get_data_size(struct vfio_device *vdev,
>  				 unsigned long *stop_copy_length)
>  {
> -	return -EOPNOTSUPP;
> +	struct ap_matrix_mdev *matrix_mdev;
> +	size_t qinfo_sz;
> +	int num_queues;
> +
> +	mutex_lock(&matrix_dev->mdevs_lock);
> +	matrix_mdev = container_of(vdev, struct ap_matrix_mdev, vdev);
> +	num_queues = vfio_ap_mdev_get_num_queues(&matrix_mdev->shadow_apcb);
> +	qinfo_sz = num_queues * sizeof(struct vfio_ap_queue_info);
> +	*stop_copy_length = qinfo_sz + sizeof(struct vfio_ap_config);
> +	mutex_unlock(&matrix_dev->mdevs_lock);
> +
> +	return 0;
>  }
>  
>  static const struct vfio_migration_ops vfio_ap_migration_ops = {

[Severity: High]
Does vfio_ap_resuming_write() leak the newly allocated ap_config and fail
subsequent writes if userspace performs a partial write during migration
resuming?

If a partial write occurs during vfio_ap_resuming_write() (i.e. *pos == 0
but len < cfg_sz), the newly allocated ap_config is not saved to
mig_data->resuming_mig_state.ap_config because the check
if (*pos + len == cfg_sz) is false.

The function then bypasses the cleanup label:

drivers/s390/crypto/vfio_ap_migration.c:vfio_ap_resuming_write() {
...
	ret = len;
	*pos += len;
	goto done;

cleanup:
	kfree(ap_config);
done:
...
}

Because the pointer was never saved in the first partial write, the
subsequent partial write fails this validation, returning -EFAULT,
which permanently fails the migration:

drivers/s390/crypto/vfio_ap_migration.c:validate_resuming_write_parms() {
...
	if ((!matrix_mdev->mig_data->resuming_mig_state.ap_config && *pos != 0) ||
	    (matrix_mdev->mig_data->resuming_mig_state.ap_config && *pos == 0)) {
		ret = -EFAULT;
...
}

[Severity: High]
Can vfio_ap_get_config() send an uninitialized config_sz field to the
migration target, causing the target to fail?

In vfio_ap_get_config(), a new struct vfio_ap_config is allocated and
populated, but its config_sz field is never initialized and remains 0.
This structure is sent verbatim over the migration stream to the target host:

drivers/s390/crypto/vfio_ap_migration.c:vfio_ap_get_config() {
...
	ap_configuration = kzalloc(*ap_config_size, GFP_KERNEL_ACCOUNT);
	if (!ap_configuration)
		return -ENOMEM;

	ap_configuration->num_queues = num_queues;

	ret = vfio_ap_store_queue_info(matrix_mdev, ap_configuration);
...
}

If the target host receives a partial write and calls reallocate_ap_config(),
it reads cur_cfg_sz = ap_config->config_sz, which is 0.

This causes the target to take the wrong reallocation path, shrinking the
buffer incorrectly:

drivers/s390/crypto/vfio_ap_migration.c:reallocate_ap_config() {
...
	cur_cfg_sz = ap_config->config_sz;
	num_queues = ap_config->num_queues;

	/* ... */
	if (cur_cfg_sz >= sizeof(struct vfio_ap_config)) {
		new_cfg_sz = calculate_ap_config_size(num_queues);
		// ...
	} else {
		new_cfg_sz = cur_cfg_sz + len;
	}
...
}

The validation if (*pos + len > cfg_sz) then fails because cfg_sz evaluates
to len and *pos > 0, causing the buffer to be freed and migration to
fail with -EIO.

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