Re: [PATCH v3 07/15] s390/vfio-ap: File ops called to save the vfio device migration state

[sashiko-bot@kernel.org]

Thank you for your contribution! Sashiko AI review found 5 potential issue(s) to consider:
- [Critical] Uninitialized config_sz in vfio_ap_config breaks migration data restoration on target
- [Critical] Concurrent read() calls race on lockless f_pos updates causing Out-of-Bounds kernel read
- [High] Dynamic state re-generation on chunked read() causes data stream corruption
- [High] Lockless release of migration file causes Use-After-Free of mig_data
- [Medium] Inverted logic for handling async hardware errors in get_hardware_info_for_queue
--

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

s390/vfio-ap: File ops called to save the vfio device migration state

This commit implements the read callback for the vfio-ap device's stop-copy
migration file. It reads hardware queue states and exposes them to userspace
for transfer to the destination host during migration.

> diff --git a/drivers/s390/crypto/vfio_ap_migration.c b/drivers/s390/crypto/vfio_ap_migration.c
> index 76e06abe5a9e9..5ef7127b697a5 100644
> --- a/drivers/s390/crypto/vfio_ap_migration.c
> +++ b/drivers/s390/crypto/vfio_ap_migration.c
> @@ -65,13 +65,6 @@ vfio_ap_release_stop_copy_file(struct vfio_ap_migration_data *mig_data)
>  		mig_data->stop_copy_mig_file = NULL;
>  }
>  
> -static ssize_t
> -vfio_ap_stop_copy_read(struct file *, char __user *, size_t, loff_t *)
> -{
> -	/* TODO */
> -	return -EOPNOTSUPP;
> -}
> -
>  static int vfio_ap_release_mig_file(struct inode *file_inode, struct file *filp)
>  {
>  	struct ap_matrix_mdev *matrix_mdev = filp->private_data;

[Severity: High]
Can this lockless release cause a use-after-free of mig_data?

In vfio_ap_release_mig_file(), matrix_mdev->mig_data is checked and then
dereferenced locklessly. Concurrently, if the device fd is closed,
vfio_ap_release_migration_data() frees matrix_mdev->mig_data and sets it to
NULL.

If a user closes the device fd while simultaneously closing the migration fd,
could this read a non-NULL pointer that is subsequently freed before being
dereferenced?

> @@ -87,6 +80,215 @@ static int vfio_ap_release_mig_file(struct inode *file_inode, struct file *filp)
>  	return 0;
>  }
>  

[ ... ]

> +static int get_hardware_info_for_queue(struct ap_matrix_mdev *matrix_mdev,
> +				       struct ap_tapq_hwinfo *hwinfo,
> +				       unsigned long apqn)
> +{
> +	struct ap_queue_status status;
> +
> +	status = ap_tapq(apqn, hwinfo);
> +
> +	switch (status.response_code) {
> +	case AP_RESPONSE_NORMAL:
> +	case AP_RESPONSE_RESET_IN_PROGRESS:
> +	case AP_RESPONSE_DECONFIGURED:
> +	case AP_RESPONSE_CHECKSTOPPED:
> +	case AP_RESPONSE_BUSY:
> +		/* For all these RCs the tapq info should be available */
> +		return 0;
> +	case AP_RESPONSE_Q_NOT_AVAIL:
> +		dev_err(matrix_mdev->vdev.dev,
> +			"migration failed: Failed to get hwinfo for queue %02lx.%04lx on target host: TAPQ rc=%d",
> +			AP_QID_CARD(apqn), AP_QID_QUEUE(apqn), status.response_code);
> +		return -ENODEV;
> +	default:
> +		/* On a pending async error the tapq info should be available */
> +		if (!status.async)
> +			return 0;

[Severity: Medium]
Is this logic inverted?

If there is no pending async error (!status.async), this returns 0 for an
unknown response code. However, it returns -EIO if there is an async error. 

The comment suggests the info should be available on a pending async error.

Should this check be if (status.async) instead?

> +
> +		dev_err(matrix_mdev->vdev.dev,
> +			"Failed to get hwinfo for queue %02lx.%04lx: TAPQ rc=%d",
> +			AP_QID_CARD(apqn), AP_QID_QUEUE(apqn), status.response_code);
> +		return -EIO;
> +	}
> +
> +	return -EINVAL;
> +}

[ ... ]

> +static int
> +vfio_ap_get_config(struct ap_matrix_mdev *matrix_mdev,
> +		   struct vfio_ap_config **ap_config, size_t *ap_config_size)
> +{
> +	struct vfio_ap_config *ap_configuration;
> +	int num_queues, ret;
> +
> +	*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;

[Severity: Critical]
Does this code fail to initialize ap_configuration->config_sz?

The target host receives this structure and relies on config_sz during
reallocate_ap_config(). If config_sz is 0, the check:

    if (cur_cfg_sz >= sizeof(struct vfio_ap_config))

falls through to:

    new_cfg_sz = cur_cfg_sz + len; // evaluates to (0 + len)

This might cause the configuration buffer to be incorrectly reallocated to
just len bytes, stripping the previously allocated space.

Will this lead to memory corruption when writing subsequent queue data?

> +
> +	ret = vfio_ap_store_queue_info(matrix_mdev, ap_configuration);
> +	if (ret) {
> +		kfree(ap_configuration);
> +		return ret;
> +	}
> +
> +	*ap_config = ap_configuration;
> +
> +	return 0;
> +}
> +
> +static ssize_t vfio_ap_stop_copy_read(struct file *filp, char __user *buf,
> +				      size_t len, loff_t *pos)
> +{
> +	struct ap_matrix_mdev *matrix_mdev;
> +	size_t ret = 0, ap_config_size;
> +	struct vfio_ap_config *ap_config;
> +
> +	/*
> +	 * When userspace calls read() with an explicit offset (pread), pos is
> +	 * non-NULL and the function rejects it with -ESPIPE (illegal seek). For
> +	 * normal read() calls, pos is NULL, so we'll use the file's internal
> +	 * position filp->f_pos
> +	 */
> +	if (pos)
> +		return -ESPIPE;
> +
> +	mutex_lock(&matrix_dev->mdevs_lock);
> +
> +	pos = &filp->f_pos;
> +
> +	ret = validate_stop_copy_read_parms(filp, pos, len);
> +	if (ret) {
> +		mutex_unlock(&matrix_dev->mdevs_lock);
> +		return ret;
> +	}
> +
> +	matrix_mdev = filp->private_data;
> +
> +	ret = vfio_ap_get_config(matrix_mdev, &ap_config, &ap_config_size);

[Severity: High]
Does this dynamically regenerate the entire device state on every read() call?

If userspace reads the data in smaller chunks (e.g., 4KB at a time), the
hardware state is re-fetched and re-allocated from scratch for every chunk.

If a queue's state or the number of queues changes on the host between
chunk reads, the generated size and data will shift. 

Could this result in torn structures, truncated streams, or duplicated data?

> +	if (ret) {
> +		mutex_unlock(&matrix_dev->mdevs_lock);
> +		return ret;
> +	}
> +
> +	/*
> +	 * If the position exceeds the size of the AP configuration data,
> +	 * then indicate EOF; otherwise calculate the length of the data to
> +	 * read such that a buffer overrun is prevented.
> +	 */
> +	if (*pos >= ap_config_size)
> +		len = 0;
> +	else
> +		len = min_t(size_t, ap_config_size - *pos, len);
> +
> +	/* If we've reached an EOF condition, let the caller know */
> +	if (len == 0) {
> +		kfree(ap_config);
> +		mutex_unlock(&matrix_dev->mdevs_lock);
> +		return 0;
> +	}
> +
> +	mutex_unlock(&matrix_dev->mdevs_lock);
> +
> +	if (copy_to_user(buf, (char *)ap_config + *pos, len)) {

[Severity: Critical]
Can concurrent read() calls race here and cause an out-of-bounds kernel
memory read?

Since this file descriptor uses FMODE_STREAM, vfs_read() bypasses f_pos_lock.
The mutex is dropped just before copy_to_user(), which directly dereferences
*pos.

If Thread A and Thread B read concurrently:

Thread A evaluates len and drops the lock.
Thread B evaluates len and drops the lock.
Thread A completes copy_to_user() and updates *pos += len.

Thread B would then evaluate (char *)ap_config + *pos using Thread A's
updated *pos. 

Could Thread B end up reading len bytes from beyond the end of the
ap_config kernel buffer?

> +		kfree(ap_config);
> +		return -EFAULT;
> +	}
> +
> +	kfree(ap_config);
> +	*pos += len;
> +	return len;
> +}

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