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

[Anthony Krowiak <akrowiak@linux.ibm.com>]

On 11/2/26 11:32 AM, sashiko-bot@kernel.org wrote:
>> 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.

Confirmed. This is the read/file-ops aspect of the same mig_data lifetime
issue noted on patches 03/14, 06/14, and 08/14.

In v12, this is resolved by decoupling mig_data deallocation from device 
close