Re: [PATCH v6 07/15] 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 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 <akrowiak@linux.ibm.com>

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?

[ ... ]

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

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?

[ ... ]

> +	/*
> +	 * 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?

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