Re: [PATCH v3 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:

Pre-existing issues:
- [Critical] Chunked writes to the resuming migration file descriptor cause a memory leak and subsequent NULL pointer dereference.
- [High] Migration config size is uninitialized on the source, breaking chunked reallocation on the destination.
--

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

s390/vfio-ap: Callback to get the size of data to be migrated during guest migration
    
Callback to get the estimated data length of the data that will be required
to complete reading of the vfio device's internal state. It's mandatory for
VFIO_DEVICE_FEATURE_MIGRATION 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: Critical]
This is a pre-existing issue, but chunked writes to the resuming migration
file descriptor can cause a memory leak and subsequent NULL pointer
dereference.

When userspace writes resuming migration data in chunks (a write where
*pos + len < cfg_sz), vfio_ap_resuming_write() successfully processes the
chunk but bypasses saving the newly allocated ap_config to mig_data, and
fails to free it:

drivers/s390/crypto/vfio_ap_migration.c:vfio_ap_resuming_write() {
...
	if (*pos == 0) {
		ret = allocate_ap_config(&ap_config, buf, len);
...
	} else {
...
	}
...
	/* Check if we've completed writing the entire configuration */
	if (*pos + len == cfg_sz) {
...
		if (!ret) {
			mig_data->resuming_mig_state.ap_config = ap_config;
			mig_data->resuming_mig_state.config_sz = cfg_sz;
		}
...
	}

	ret = len;
	*pos += len;
	goto done;

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

If the first write is a partial chunk, ap_config is allocated but the
function skips assigning it to mig_data->resuming_mig_state.ap_config and
jumps to done, leaking ap_config.

On the next chunk write (*pos > 0), the else branch reads
mig_data->resuming_mig_state.ap_config which is still NULL. It passes it to
reallocate_ap_config(), where it is unconditionally dereferenced:

drivers/s390/crypto/vfio_ap_migration.c:reallocate_ap_config() {
	struct vfio_ap_config *ap_config = *mig_ap_config;
...
	cur_cfg_sz = ap_config->config_sz;

Can this sequence cause a memory leak on partial writes, followed by a NULL
pointer dereference on the next chunk write?

[Severity: High]
This is a pre-existing issue, but the migration config size appears to be
left uninitialized on the source, which can break chunked reallocation on
the destination.

In vfio_ap_get_config(), ap_configuration is allocated via kzalloc but the
config_sz field is never populated:

drivers/s390/crypto/vfio_ap_migration.c:vfio_ap_get_config() {
...
	*ap_config_size = vfio_ap_config_size(matrix_mdev, &num_queues);

	ap_configuration = kzalloc(*ap_config_size, GFP_KERNEL_ACCOUNT);
	if (!ap_configuration)
		return -ENOMEM;

	ap_configuration->num_queues = num_queues;
...
}

Because config_sz remains 0, the source sends a zeroed config_sz to
userspace. On the destination, vfio_ap_resuming_write() copies this
structure from userspace. If a subsequent chunk triggers
reallocate_ap_config(), it reads the 0 size and reallocates the buffer to
merely len. This causes the subsequent validation:

	if (*pos + len > cfg_sz) {
		ret = -EIO;
		goto cleanup;
	}

to evaluate as true (since *pos + len > len is true when *pos > 0),
aborting the migration with -EIO.

Should config_sz be initialized in vfio_ap_get_config() before it is sent
to the destination?

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