Re: [PATCH v5 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 5 potential issue(s) to consider:
- [High] Concurrent `read()` calls trigger a TOCTOU race condition on unprotected `filp->f_pos`, leading to data corruption and a heap out-of-bounds read.
- [High] A TOCTOU race condition in `vfio_ap_get_config()` permanently leaks the `ap_configuration` buffer during concurrent reads.
- [High] `__counted_by` bounds violation panic due to accessing a flexible array before initializing its count.
- [High] Use-After-Free of `matrix_mdev` because the migration file descriptor does not hold a reference to the VFIO device.
- [Medium] Resource exhaustion and high allocation failure probability due to order-8 memory allocation on every read chunk.
--

--- Patch [7]: [PATCH v5 07/15] s390/vfio-ap: File ops called to save the vfio device migration state ---
commit ea39a12de8ce91ab4c204140075df7f484f9c8f8
Author: Anthony Krowiak <akrowiak@linux.ibm.com>
Subject: s390/vfio-ap: File ops called to save the vfio device migration state

This commit implements the read callback for the migration stop-copy state
to transfer the guest's AP configuration and queue information to userspace.

> diff --git a/drivers/s390/crypto/vfio_ap_migration.c b/drivers/s390/crypto/vfio_ap_migration.c
> index e566250a88ea3..0d949ec205de6 100644
> --- a/drivers/s390/crypto/vfio_ap_migration.c
> +++ b/drivers/s390/crypto/vfio_ap_migration.c

[ ... ]

> +static int validate_stop_copy_read_parms(struct file *filp, loff_t *pos,
> +					 size_t len)
> +{
> +	struct vfio_ap_migration_data *mig_data;
> +	struct ap_matrix_mdev *matrix_mdev;
> +	loff_t total_len;
> +
> +	lockdep_assert_held(&matrix_dev->mdevs_lock);
> +
> +	if (check_add_overflow((loff_t)len, *pos, &total_len))
> +		return -EIO;
> +
> +	matrix_mdev = filp->private_data;
> +
> +	if (!matrix_mdev || !matrix_mdev->mig_data)
> +		return -ENODEV;

[Severity: High]
Does the migration file descriptor hold a reference to the VFIO device?

If userspace closes the main VFIO device file descriptor but keeps the
migration file descriptor open, the VFIO core could unregister and free the
vfio_device and the embedding matrix_mdev. A subsequent read() on the
migration file would then dereference the freed matrix_mdev here.

[ ... ]

> +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;
> +
> +	/*
> +	 * Snapshot the APQN list from shadow_apcb under the lock so that
> +	 * ap_tapq() calls in vfio_ap_store_queue_info() can happen without it.
> +	 */
> +	apm = matrix_mdev->shadow_apcb.apm;
> +	aqm = matrix_mdev->shadow_apcb.aqm;
> +	num_queues = 0;
> +	for_each_set_bit_inv(apid, apm, AP_DEVICES) {
> +		for_each_set_bit_inv(apqi, aqm, AP_DOMAINS) {
> +			ap_configuration->qinfo[num_queues].apqn =
> +				AP_MKQID(apid, apqi);
> +			num_queues += 1;
> +		}
> +	}
> +	ap_configuration->num_queues = num_queues;

[Severity: High]
Will this trigger a __counted_by bounds check panic?

The struct vfio_ap_config uses __counted_by(num_queues) for the qinfo
flexible array. Since kzalloc() initializes num_queues to 0, writing to
qinfo[num_queues] before assigning ap_configuration->num_queues = num_queues
causes compiler-inserted bounds checks to evaluate the access against a
count of 0.

> +	memcpy(ap_configuration->adm, matrix_mdev->shadow_apcb.adm,
> +	       sizeof(ap_configuration->adm));
> +	mdev_name = dev_name(matrix_mdev->vdev.dev);
> +
> +	/**
> +	 * Release the global mdevs_lock which guards access to all mdevs.
> +	 * Storing the queue information could take a while if there are a
> +	 * large number of queues.
> +	 */
> +	mutex_unlock(&matrix_dev->mdevs_lock);
> +
> +	ret = vfio_ap_store_queue_info(mdev_name, ap_configuration);
> +	if (ret) {
> +		kfree(ap_configuration);
> +		mutex_lock(&matrix_dev->mdevs_lock);
> +		return ret;
> +	}
> +
> +	/* Retake the mdevs lock so we can safely make updates to the mdev */
> +	mutex_lock(&matrix_dev->mdevs_lock);
> +
> +	/*
> +	 * Verify mig_data is still valid - mdevs_lock was dropped, so the
> +	 * device could have been closed concurrently.
> +	 */
> +	if (!matrix_mdev->mig_data) {
> +		kfree(ap_configuration);
> +		return -ENODEV;
> +	}
> +
> +	matrix_mdev->mig_data->stop_copy_mig_file.ap_config = ap_configuration;

[Severity: High]
Does dropping mdevs_lock allow concurrent readers to leak ap_configuration?

In vfio_ap_stop_copy_read(), if stop_copy_mig_file.ap_config is NULL,
it calls this function. Because mdevs_lock is temporarily dropped here for
the hardware query, a concurrent read() could also observe that ap_config is
NULL, enter this function, and allocate a second buffer.

When the lock is reacquired, both threads will sequentially overwrite
stop_copy_mig_file.ap_config, permanently leaking the previous buffer.

[ ... ]

> +static ssize_t vfio_ap_stop_copy_read(struct file *filp, char __user *buf,
> +				      size_t len, loff_t *pos)
> +{
> +	struct ap_matrix_mdev *matrix_mdev;
> +	struct vfio_ap_config *ap_config;
> +	ssize_t ret = 0;
> +	size_t ap_config_size;

[ ... ]

> +	/*
> +	 * Allocate ap_config and copy the content of the object caching the
> +	 * AP configuration data between calls to it so we can give up the
> +	 * global mdevs_lock while copying the data to userspace.
> +	 */
> +	ap_config = kzalloc(ap_config_size, GFP_KERNEL_ACCOUNT);

[Severity: Medium]
Is it safe to allocate up to 1MB using kzalloc() on every read syscall?

ap_config_size can exceed 1MB, requiring a contiguous order-8 allocation.
Requesting this on every read() regardless of the requested chunk length
is likely to fail with -ENOMEM on fragmented systems, breaking migration.
It may also allow userspace to cause memory allocator thrashing by issuing
small, repetitive reads.

> +	if (!ap_config) {
> +		mutex_unlock(&matrix_dev->mdevs_lock);
> +		return -ENOMEM;
> +	}
> +
> +	memcpy(ap_config, matrix_mdev->mig_data->stop_copy_mig_file.ap_config,
> +	       ap_config_size);

[Severity: High]
Will this memcpy() also trigger a __counted_by panic?

Since ap_config was just allocated with kzalloc(), its num_queues field is 0.
The memcpy() relies on __struct_size(), which will evaluate only to the base
structure size without the flexible array elements, causing a buffer
overflow panic during the copy.

> +
> +	/*
> +	 * Give up the global mdevs_lock while copying data to the user; it
> +	 * might be a long running operation and we don't want to prevent access
> +	 * to other mdevs for an inordinate amount of time.
> +	 */
> +	mutex_unlock(&matrix_dev->mdevs_lock);
> +
> +	if (copy_to_user(buf, (char *)ap_config + *pos, len)) {
> +		kfree(ap_config);
> +		return -EFAULT;
> +	}
> +
> +	kfree(ap_config);
> +	*pos += len;

[Severity: High]
Can concurrent read() calls cause an out-of-bounds heap read here?

Because the file is opened with FMODE_STREAM, VFS does not serialize reads
with f_pos_lock. The read length is calculated based on *pos under
mdevs_lock, but the lock is dropped before copy_to_user() and the offset
increment (*pos += len).

A concurrent read() can calculate its length based on an outdated *pos,
wait for the lock, and then read from ap_config + *pos after the first
thread has advanced *pos. This would allow reading past the end of the
ap_config buffer, leaking adjacent kernel memory to userspace.

> +
> +	return len;
> +}

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