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

Thank you for your contribution! Sashiko AI review found 3 potential issue(s) to consider:
- [Critical] Dropping `mdevs_lock` before `copy_to_user()` in `vfio_ap_stop_copy_read` exposes `mig_data` and its `ap_config` buffer to concurrent frees, leading to a Use-After-Free and kernel memory leak.
- [High] Concurrent reads from the stop_copy file race to initialize `ap_config` because `mdevs_lock` is temporarily dropped in `vfio_ap_get_config()`, causing memory leaks of dynamically allocated configuration buffers.
- [High] Caching the local `mig_file` pointer before dropping `mdevs_lock` in `vfio_ap_get_config()` leads to a Use-After-Free if `mig_data` is reallocated.
--

--- Patch [7]: [PATCH v7 07/15] s390/vfio-ap: File ops called to save the vfio device migration state ---
commit 373ad9ea7be2af5d58b1a74f7797578d1528e3b7
Author: Anthony Krowiak <[email protected]>

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

This patch implements the read callback for the vfio-ap device migration
state file. It copies the guest's AP configuration and hardware information
to userspace when the migration state transitions to STOP_COPY. This state
data will then be transferred to the destination host.

> diff --git a/drivers/s390/crypto/vfio_ap_migration.c b/drivers/s390/crypto/vfio_ap_migration.c
> index de693b308925d..50781b61f7f18 100644
> --- a/drivers/s390/crypto/vfio_ap_migration.c
> +++ b/drivers/s390/crypto/vfio_ap_migration.c
[ ... ]
> @@ -150,6 +144,279 @@ static int vfio_ap_release_mig_file(struct inode *file_inode, struct file *filp)
>  	return 0;
>  }
[ ... ]
> +static int vfio_ap_get_config(struct ap_matrix_mdev *matrix_mdev)
> +{
[ ... ]
> +	ap_configuration = kvzalloc(ap_config_size, GFP_KERNEL_ACCOUNT);
> +	if (!ap_configuration)
> +		return -ENOMEM;
[ ... ]
> +	/*
> +	 * Unlock the mdevs_lock so other mdevs are not precluded from being
> +	 * accessed while a potentially long running operation is performed.
> +	 */
> +	mutex_unlock(&matrix_dev->mdevs_lock);
> +	ret = vfio_ap_store_queue_info(mdev_name, ap_configuration);
> +	mutex_lock(&matrix_dev->mdevs_lock);
> +	if (ret) {
> +		kvfree(ap_configuration);
> +		return ret;
> +	}
> +
> +	if (!matrix_mdev->mig_data) {
> +		kvfree(ap_configuration);
> +		return -ENODEV;
> +	}
> +
> +	matrix_mdev->mig_data->stop_copy_mig_file.ap_config = ap_configuration;

[Severity: High]
Can two concurrent reads cause a memory leak here?

Since mdevs_lock is temporarily dropped above, both threads might observe
that ap_config is NULL in vfio_ap_stop_copy_read(), and both would allocate
an ap_configuration buffer. 

When they reacquire the lock here, they will both blindly overwrite
matrix_mdev->mig_data->stop_copy_mig_file.ap_config with their respective
allocations, permanently leaking the buffer allocated by the first thread.

> +	matrix_mdev->mig_data->stop_copy_mig_file.config_sz = ap_config_size;
> +
> +	return 0;
> +}
> +
> +static ssize_t vfio_ap_stop_copy_read(struct file *filp, char __user *buf,
> +				      size_t len, loff_t *pos)
> +{
[ ... ]
> +	matrix_mdev = filp->private_data;
> +	mig_file = &matrix_mdev->mig_data->stop_copy_mig_file;
> +
> +	if (!mig_file->ap_config) {
> +		ret = vfio_ap_get_config(matrix_mdev);

[Severity: High]
Could the cached mig_file pointer become stale and cause a use-after-free?

Because vfio_ap_get_config() temporarily drops mdevs_lock, a concurrent
thread could potentially free and reallocate the mig_data structure. When
vfio_ap_get_config() returns, the stack-cached mig_file pointer might still
point to the old, freed mig_data structure.

> +		if (ret) {
> +			mutex_unlock(&matrix_dev->mdevs_lock);
> +			return ret;
> +		}
> +	}
> +
> +	/*
> +	 * Compute the offset and clamped length fully under the lock so that
> +	 * concurrent read()s on this stream file each see a consistent view of
> +	 * the current position.  *pos is advanced here while we still hold the
> +	 * lock; copy_to_user() then uses the snapshot read_pos.  This prevents
> +	 * two threads from calculating the same offset and both copying the
> +	 * same region (or one reading past the end of the buffer).
> +	 */
> +	if (*pos >= mig_file->config_sz) {
> +		mutex_unlock(&matrix_dev->mdevs_lock);
> +		return 0;
> +	}
> +
> +	len = min_t(size_t, mig_file->config_sz - *pos, len);
> +	if (len == 0) {
> +		mutex_unlock(&matrix_dev->mdevs_lock);
> +		return 0;
> +	}
> +
> +	read_pos = *pos;
> +	*pos += len;
> +
> +	/*
> +	 * Drop the lock only for the copy_to_user().  The ap_config buffer is
> +	 * stable: it is allocated once in vfio_ap_get_config() and freed only
> +	 * in vfio_ap_release_stop_copy_file() which requires mdevs_lock.
> +	 * Since we already advanced *pos above, no other thread will compute an
> +	 * overlapping region.
> +	 */
> +	mutex_unlock(&matrix_dev->mdevs_lock);
> +
> +	if (copy_to_user(buf, (char *)mig_file->ap_config + read_pos, len))

[Severity: Critical]
Is it safe to access mig_file and its ap_config here without the lock?

Once mdevs_lock is dropped, a concurrent thread could close the VFIO device
or issue a reset, which acquires mdevs_lock and frees both the ap_config
buffer and the mig_data struct. 

If that happens before or during copy_to_user(), it could result in reading
freed kernel memory and leaking it to userspace, or crashing the kernel.

> +		return -EFAULT;
> +
> +	return len;
> +}

-- 
Sashiko AI review · https://sashiko.dev/#/patchset/[email protected]?part=7