Re: [PATCH v10 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 2 potential issue(s) to consider:
- [Medium] s390/vfio-ap: f_pos corruption on -EFAULT in vfio_ap_stop_copy_read()
- [Medium] s390/vfio-ap: high-order kmalloc() in vfio_ap_stop_copy_read()
--

commit 244125b3df6e4802b043acebf35691558c6264c3
Author: Anthony Krowiak <akrowiak@linux.ibm.com>

s390/vfio-ap: File ops called to save the vfio device migration state

This commit implements the vfio_ap_stop_copy_read() callback for the
migration file created when transitioning from stop to stop-copy state,
copying the guest's AP configuration and queue hardware information to
userspace.

> diff --git a/drivers/s390/crypto/vfio_ap_migration.c b/drivers/s390/crypto/vfio_ap_migration.c
> index bc216c51a1a79..f9b00fb89410f 100644
> --- a/drivers/s390/crypto/vfio_ap_migration.c
> +++ b/drivers/s390/crypto/vfio_ap_migration.c

[ ... ]

> @@ -162,6 +155,269 @@ static int vfio_ap_release_mig_file(struct inode *file_inode, struct file *filp)

[ ... ]

> +static int vfio_ap_get_config(struct ap_matrix_mdev *matrix_mdev)
> +{
> +	struct vfio_ap_config *ap_configuration;
> +	unsigned long *apm, *aqm, apid, apqi;
> +	unsigned int num_queues;
> +	const char *mdev_name;
> +	size_t ap_config_size;
> +	int ret, qindex;
> +
> +	lockdep_assert_held(&matrix_dev->mdevs_lock);
> +
> +	ap_config_size = vfio_ap_config_size(matrix_mdev, &num_queues);
> +
> +	ap_configuration = kvzalloc(ap_config_size, GFP_KERNEL_ACCOUNT);
> +	if (!ap_configuration)
> +		return -ENOMEM;

[ ... ]

> +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;
> +	void *bounce;
> +	ssize_t ret;

[ ... ]

> +	/*
> +	 * Don't read past the end of the AP configuration data. Limit len to
> +	 * the number of bytes remaining.
> +	 */
> +	len = min_t(size_t, mig_file->config_sz - *pos, len);
> +	if (len == 0) {
> +		mutex_unlock(&matrix_dev->mdevs_lock);
> +		return 0;
> +	}
> +
> +	/*
> +	 * Copy into a bounce buffer under mdevs_lock and advance *pos, then
> +	 * perform copy_to_user() outside the lock. This avoids stalling
> +	 * all host mdev operations if userspace passes a buffer that faults.
> +	 */
> +	bounce = kmalloc(len, GFP_KERNEL);
> +	if (!bounce) {
> +		mutex_unlock(&matrix_dev->mdevs_lock);
> +		return -ENOMEM;
> +	}

[Severity: Medium]
In drivers/s390/crypto/vfio_ap_migration.c, vfio_ap_get_config() allocates
ap_configuration using kvzalloc() with GFP_KERNEL_ACCOUNT because a guest's
configuration can hold up to 65,536 queues, making mig_file->config_sz up to
1,048,624 bytes:

	ap_config_size = vfio_ap_config_size(matrix_mdev, &num_queues);

	ap_configuration = kvzalloc(ap_config_size, GFP_KERNEL_ACCOUNT);

In vfio_ap_stop_copy_read(), len is clamped only to mig_file->config_sz - *pos
and passed directly to kmalloc(). When userspace reads the full migration
blob in a single read() call for a guest with a large queue configuration,
this requests up to an order-9 (2 MiB) physically contiguous allocation
without vmalloc fallback or memcg accounting, which can fail and trigger page
allocation warnings on fragmented hosts.

Should vfio_ap_stop_copy_read() allocate bounce with kvmalloc() using
GFP_KERNEL_ACCOUNT and free it with kvfree() instead of kmalloc() and
kfree()?

> +
> +	memcpy(bounce, (char *)mig_file->ap_config + *pos, len);
> +	*pos += len;
> +	mutex_unlock(&matrix_dev->mdevs_lock);
> +
> +	if (copy_to_user(buf, bounce, len)) {
> +		kfree(bounce);
> +		return -EFAULT;
> +	}

[Severity: Medium]
What happens to filp->f_pos if copy_to_user() fails in
vfio_ap_stop_copy_read()?

In drivers/s390/crypto/vfio_ap_migration.c, vfio_ap_stop_copy_read() sets
pos = &filp->f_pos, advances *pos += len, and drops matrix_dev->mdevs_lock
before calling copy_to_user(). If copy_to_user() faults and returns non-zero,
the function returns an error while leaving filp->f_pos permanently advanced
past the unread bytes.

Because vfio_ap_open_file_stream() opens the migration file with
stream_open(), seeking and pread() are rejected, so userspace cannot rewind
the stream. Any subsequent read() will silently skip those len bytes or
observe end-of-file prematurely, corrupting or truncating the saved migration
state. In addition, because stream_open() clears atomic position locking on
filp->f_mode, dropping matrix_dev->mdevs_lock after advancing *pos but before
copy_to_user() allows concurrent readers to advance *pos further, preventing
a safe rollback and allowing out-of-order stream delivery.

Can *pos be updated only after copy_to_user() succeeds (or only by the number
of bytes actually copied), while keeping concurrent reads serialized?

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