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

On 7/27/26 2:02 PM, [email protected] wrote:
> Thank you for your contribution! Sashiko AI review found 4 potential issue(s) to consider:
> - [Critical] Pointer casting a 64-bit uninitialized `unsigned long` to a 32-bit `int *` on a big-endian architecture leaves the lower 32 bits uninitialized, leading to massive buffer overflows or a `__counted_by` trap.
> - [High] Dropping `mdevs_lock` before `copy_to_user()` opens a TOCTOU race window that allows concurrent freeing of the `ap_config` buffer and `mig_data`, causing a double Use-After-Free.
> - [High] The global `mdevs_lock` is held across potentially tens of thousands of slow hardware instructions, explicitly contradicting documented subsystem design and risking host soft lockups.
> - [High] The `.read` callback unconditionally rejects all standard read operations because it erroneously assumes the `pos` pointer is NULL.
> --
>
> --- Patch [7]: [PATCH v6 07/15] s390/vfio-ap: File ops called to save the vfio device migration state ---
> commit 6828e02e20841803a7c3d24379ed9d9979222d55
> Author: Anthony Krowiak <[email protected]>
>
> s390/vfio-ap: File ops called to save the vfio device migration state
>
> This commit implements the read callback function for the vfio-ap device
> migration state file. It copies the guest's AP configuration and hardware
> queue information to userspace when the device transitions to the STOP_COPY
> state.
>
>> --- a/drivers/s390/crypto/vfio_ap_migration.c
>> +++ b/drivers/s390/crypto/vfio_ap_migration.c
> [ ... ]
>
>> +static int vfio_ap_store_queue_info(const char *mdev_name,
>> +				    struct vfio_ap_config *ap_config)
>> +{
>> +	struct ap_tapq_hwinfo source_hwinfo;
>> +	unsigned long num_queues;
>> +	int ret;
>> +
>> +	/*
>> +	 * ap_tapq() is a hardware instruction that may take time to complete.
>> +	 * It must be called without mdevs_lock held to avoid blocking other
>> +	 * mdevs. The apqn list was already snapshotted into ap_config->qinfo[]
>> +	 * by the caller under the lock.
>> +	 */
>> +	for (num_queues = 0; num_queues < ap_config->num_queues; num_queues++) {
>> +		ret = get_hardware_info_for_queue(mdev_name, &source_hwinfo,
>> +						  ap_config->qinfo[num_queues].apqn);
> [Severity: High]
> Does this loop actually execute with the mdevs_lock held, despite the
> comment?
>
> Looking at the call chain, vfio_ap_stop_copy_read() acquires
> matrix_dev->mdevs_lock and calls vfio_ap_get_config(), which asserts
> the lock is held and then calls vfio_ap_store_queue_info().
>
> Since the lock is never dropped in this chain, will executing PQAP(TAPQ)
> in a loop for potentially many queues block other mdev operations across
> the host?

The mdevs_lock will be dropped prior by the caller of this function.

>
> [ ... ]stream_open is used by subsystems that want stream-like file descriptors.
>   * Such file descriptors are not seekable and don't have notion of position
>   * (file.f_pos is always 0 and ppos passed to .read()/.write() is always NULL)
>
>> +static int vfio_ap_get_config(struct ap_matrix_mdev *matrix_mdev)
>> +{
>> +	unsigned long *apm, *aqm, apid, apqi, num_queues;
>> +	struct vfio_ap_config *ap_configuration;
>> +	const char *mdev_name;
>> +	size_t ap_config_size;
>> +	int ret;
>> +
>> +	lockdep_assert_held(&matrix_dev->mdevs_lock);
>> +
>> +	ap_config_size = vfio_ap_config_size(matrix_mdev, (int *)&num_queues);
>> +
>> +	ap_configuration = kzalloc(ap_config_size, GFP_KERNEL_ACCOUNT);
>> +	if (!ap_configuration)
>> +		return -ENOMEM;
>> +
>> +	/*
>> +	 * num_queues must be set before writing qinfo[] elements; the
>> +	 * __counted_by(num_queues) annotation on qinfo[] causes the compiler to
>> +	 * insert bounds checks that evaluate against ap_configuration->num_queues.
>> +	 * Writing through qinfo[i] with num_queues still 0 would trap.
>> +	 */
>> +	ap_configuration->num_queues = num_queues;
> [Severity: Critical]
> Can casting the uninitialized 8-byte unsigned long num_queues to an
> int pointer cause uninitialized memory issues on big-endian systems?

The cast of num_queues to (int * ) needs to be removed.
The vfio_ap_config_size signature takes int *num_queues, and the variable
is already int. On big-endian s390x, &num_queues now points directly to
the correctly-sized storage that vfio_ap_config_size writes into.

>
> On s390x, writing a 32-bit integer to an 8-byte address only initializes
> the most significant 4 bytes, leaving the least significant 4 bytes as
> stack garbage.
>
> When ap_configuration->num_queues (a u32) is assigned num_queues, will it
> truncate the 8-byte value, completely discarding the initialized high
> bytes and storing the uninitialized garbage data? Could this lead to
> out-of-bounds accesses in the array bounds checks?
>
> [ ... ]
>
>> +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;
>> +	struct ap_matrix_mdev *matrix_mdev;
>> +	loff_t read_pos;
>> +	ssize_t ret;
>> +
>> +	/*
>> +	 * When userspace calls read() with an explicit offset (pread), pos is
>> +	 * non-NULL and the function rejects it with -ESPIPE (illegal seek). For
>> +	 * normal read() calls, pos is NULL, so we'll use the file's internal
>> +	 * position filp->f_pos
>> +	 */
>> +	if (pos)
>> +		return -ESPIPE;
> [Severity: High]
> Is the assumption that pos is NULL for normal read() calls correct?
>
> The VFS layer unconditionally passes a valid, non-NULL loff_t pointer
> (pointing to a local stack variable) to the .read callback for standard
> reads. Will this check unconditionally reject all standard read operations
> on the migration state file?

The vfio_ap_open_file_stream calls the stream_open function to open
the file stream which is passed into the vfio_ap_stop_copy_read
function. The function comments for stream_open state:

"streamopen is used by subsystems that want stream-like file
descriptors. Such file descriptors are not seekable and don't have
notion of position (file.f_pos is always 0 and ppos passed to
.read()/.write() is always NULL.

>
> [ ... ]
>
>> +	/*
>> +	 * 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_mig_files() / vfio_ap_release_stop_copy_file(),
>> +	 * both of which require 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))
>> +		return -EFAULT;
>> +
>> +	return len;
>> +}
> [Severity: High]
> Could dropping mdevs_lock here open a race condition that leads to a
> use-after-free?
>
> If a concurrent thread triggers a device close or reset (for example,
> via VFIO_DEVICE_RESET) while this thread is at the copy_to_user() call,
> the reset path could acquire mdevs_lock and free both the ap_config
> buffer and the mig_data struct.
>
> Since mig_file is a pointer into mig_data, will dereferencing it here to
> access ap_config cause a double use-after-free, copying freed memory to
> userspace?

The use-after-free cannot occur because all paths that free ap_config or
mig_data require mdevs_lock, and the lock is held for the entire window
except the copy_to_user. mig_data itself (which mig_file is embedded
in) is also only freed under the lock, so the mig_file pointer remains
valid across the unlock.

>