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

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

On 8/7/26 6:37 PM, sashiko-bot@kernel.org wrote:

All three findings were consequences of the lock-drop around 
|vfio_ap_store_queue_info()|and the 
unlock-before-|copy_to_user()|pattern, both of which were removed.

>> 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

All three findings below are consequences of the lock-drop around
vfio_ap_store_queue_info() and the unlock before copy_to_user()
pattern. The mdevs_lock will be held for the duration of the
vfio_ap_stop_copy_read() function in the next version of
the series.

> [ ... ]
>> @@ -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;
>> +}