Re: [PATCH v11 07/14] 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 1 potential issue(s) to consider:
- [Critical] s390/vfio-ap: use-after-free of mig_file in vfio_ap_stop_copy_read()
--

commit 6f641fc7194e9bced026956cec2ed8bc278c6d77
Author: Anthony Krowiak <akrowiak@linux.ibm.com>

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

This commit implements the vfio_ap_stop_copy_read() callback for the migration
file created when transitioning from STOP to STOP_COPY, snapshotting and
copying the guest's AP configuration and queue hardware information to
userspace.

> diff --git a/drivers/s390/crypto/vfio_ap_migration.c b/drivers/s390/crypto/vfio_ap_migration.c
> index 48028b12b26f9..b64ce092a41b1 100644
> --- a/drivers/s390/crypto/vfio_ap_migration.c
> +++ b/drivers/s390/crypto/vfio_ap_migration.c

[ ... ]

> @@ -167,6 +160,269 @@ static int vfio_ap_release_mig_file(struct inode *file_inode, struct file *filp)
>  	return 0;
>  }
>  
> +/**
> + * validate_stop_copy_read_parms: Validate the input parameters to the
> + *                                vfio_ap_stop_copy_read function
> + *
> + * @mig_file:	Pointer to the object used for storing the vfio-ap device state
> + * @filp:	File stream to which to write source guest's AP configuration
> + * @len:	The length of the data to be read
> + *
> + * Verify the following:
> + * - @filp is the instance opened when state transitioned from STOP to STOP_COPY
> + * - @filp->f_pos + @len does not cause integer overflow
> + * - @mig_file->matrix_mdev is not NULL
> + * - @mig_file->matrix_mdev->mig_data is not NULL
> + *
> + * Returns: 0 if the parameters pass validation; otherwise returns an error
> + */
> +static int validate_stop_copy_read_parms(struct vfio_ap_migration_file *mig_file,
> +					 struct file *filp,
> +					 size_t len)
> +{
> +	loff_t total_len;
> +
> +	lockdep_assert_held(&mig_file->lock);
> +	lockdep_assert_held(&matrix_dev->mdevs_lock);
> +
> +	if (filp != mig_file->filp)
> +		return -EINVAL;
> +
> +	if (check_add_overflow((loff_t)len, filp->f_pos, &total_len))
> +		return -EIO;
> +
> +	if (!mig_file->matrix_mdev || !mig_file->matrix_mdev->mig_data)
> +		return -ENODEV;
> +
> +	return 0;
> +}

[ ... ]

> +static ssize_t vfio_ap_stop_copy_read(struct file *filp, char __user *buf,
> +				      size_t len, loff_t *pos)
> +{
> +	struct vfio_ap_migration_file *mig_file;
> +	void *bounce;
> +	ssize_t ret;
> +
> +	/*
> +	 * This file was opened with stream_open(), so pos should be NULL for
> +	 * sequential read() calls; a non-NULL pointer will be passed only
> +	 * for positional pread() calls in which case we return an error
> +	 * indicating broken pipe/illegal seek on a non-seekable file
> +	 */
> +	if (pos)
> +		return -ESPIPE;
> +
> +	/*
> +	 * mig_file is guaranteed live here: vfio_ap_open_file_stream() took
> +	 * a vfio_device registration reference that is held until
> +	 * vfio_ap_release_mig_file() runs, so the embedding mig_file cannot
> +	 * be freed while this file descriptor is open.
> +	 */
> +	mig_file = filp->private_data;
> +	mutex_lock(&mig_file->lock);
> +	mutex_lock(&matrix_dev->mdevs_lock);
> +
> +	ret = validate_stop_copy_read_parms(mig_file, filp, len);
> +	if (ret) {
> +		mutex_unlock(&matrix_dev->mdevs_lock);
> +		mutex_unlock(&mig_file->lock);
> +		return ret;
> +	}

[ ... ]

> +	memcpy(bounce, (char *)mig_file->ap_config + *pos, len);
> +	mutex_unlock(&matrix_dev->mdevs_lock);
> +
> +	if (copy_to_user(buf, bounce, len)) {
> +		kfree(bounce);
> +		mutex_unlock(&mig_file->lock);
> +		return -EFAULT;
> +	}
> +
> +	*pos += len;
> +	mutex_unlock(&mig_file->lock);
> +	kfree(bounce);
> +	return len;
> +}

[Severity: Critical]
As reported by Sashiko on v6 [1], can closing the vfio device file descriptor
while the migration file descriptor remains open cause a use-after-free of
mig_file in vfio_ap_stop_copy_read() and vfio_ap_release_mig_file()?

The comment in vfio_ap_stop_copy_read() notes that vfio_ap_open_file_stream()
takes a vfio_device registration reference via
vfio_device_try_get_registration() until vfio_ap_release_mig_file() runs.
However, that reference only keeps matrix_mdev alive; stop_copy_mig_file is
embedded inside struct vfio_ap_migration_data (matrix_mdev->mig_data), which
is allocated separately in vfio_ap_init_migration_data().

When userspace closes the vfio device file descriptor,
vfio_ap_mdev_close_device() -> vfio_ap_release_migration_data() runs under
matrix_dev->mdevs_lock:

drivers/s390/crypto/vfio_ap_migration.c:vfio_ap_release_migration_data() {
	if (!matrix_mdev->mig_data)
		return;

	vfio_ap_release_mig_files(matrix_mdev);
	mutex_destroy(&matrix_mdev->mig_data->stop_copy_mig_file.lock);
	mutex_destroy(&matrix_mdev->mig_data->resuming_mig_file.lock);
	kfree(matrix_mdev->mig_data);
	matrix_mdev->mig_data = NULL;
}

Because filp->private_data on the still-open migration file descriptor points
to &mig_data->stop_copy_mig_file, freeing matrix_mdev->mig_data frees
mig_file itself:

1. If vfio_ap_stop_copy_read() runs after vfio_ap_mdev_close_device(),
   mig_file = filp->private_data points into freed memory.
   mutex_lock(&mig_file->lock) operates on a destroyed mutex in freed slab
   memory, and validate_stop_copy_read_parms() dereferences mig_file->filp and
   mig_file->matrix_mdev from freed memory before checking
   !mig_file->matrix_mdev->mig_data.

2. If vfio_ap_stop_copy_read() is already in progress and drops
   matrix_dev->mdevs_lock before copy_to_user() while holding mig_file->lock
   (or is waiting to acquire matrix_dev->mdevs_lock after locking
   mig_file->lock), a concurrent vfio_ap_mdev_close_device() ->
   vfio_ap_release_migration_data() will destroy mig_file->lock while it is
   locked and kfree(matrix_mdev->mig_data), causing the subsequent
   mutex_unlock(&mig_file->lock) in vfio_ap_stop_copy_read() to write to freed
   memory.

3. When the migration file descriptor is later closed,
   vfio_ap_release_mig_file() dereferences mig_file->matrix_mdev from the
   freed mig_data structure.

[1] https://lore.kernel.org/r/20260727180224.027021F000E9@smtp.kernel.org

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