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

On 8/7/26 6:37 PM, [email protected] wrote:
> 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)
>> +{The scenario requires two read() calls to simultaneously observe ap_config == NULL and both enter vfio_ap_get_config(). This cannot happen because:
>>
>> The fd is opened with stream_open(), which serializes read calls on the same fd.
>> Even if two independent fds existed (they don't — one fd per STOP_COPY transition), both callers still serialize on mdevs_lock. The second to acquire the lock will find ap_config already populated and skip the allocation.
> [ ... ]
>> +	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.

The scenario requires two read() calls to simultaneously observe
ap_config == NULL and both enter vfio_ap_get_config(). This cannot happen
because:

1. The fd is opened with stream_open(), which serializes read calls on the
    same fd.
2. Even if two independent fds existed (they don't — one fd per
    STOP_COPY transition), both callers still serialize on mdevs_lock. The
    second to acquire the lock will find ap_config already populated and
    skip the allocation.

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

The concern that mig_data is freed while the lock is dropped inside
vfio_ap_get_config(), making the pointer stale when
vfio_ap_stop_copy_read() uses it afterward is unfounded:

1. All callers that free or NULL out mig_data —
    vfio_ap_release_migration_data() and vfio_ap_reset_migration_state() -
    hold mdevs_lock, same as the unlock/relock pair inside
    vfio_ap_get_config().

2. vfio_ap_get_config() already re-checks matrix_mdev->mig_data after
    re-acquiring the lock and returns -ENODEV before touching anything if it
    was freed. That return propagates up to vfio_ap_stop_copy_read(), which
    exits before the stale mig_file is accessed.

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

The code dropped mdevs_lock before copy_to_user(), but
vfio_ap_reset_migration_state() (e.g. triggered by VFIO_DEVICE_RESET while
the stop-copy fd is still open) calls vfio_ap_release_stop_copy_file()
under the lock, which kvfree()s the buffer. A concurrent copy_to_user()
outside the lock would then read freed kernel memory.

Since the ap_config data is at most a few KB for any realistic AP
configuration, the fix would be to hold the lock while the
copy_from_user is executing. This is safe to do under a sleeping mutex.

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