Re: [PATCH v6 10/15] s390/vfio-ap: File ops called to resume the vfio device migration

[sashiko-bot@kernel.org]

Thank you for your contribution! Sashiko AI review found 4 potential issue(s) to consider:
- [Critical] Double fetch vulnerability in the resuming write path allows out-of-bounds kernel reads.
- [Critical] Use-After-Free during concurrent device teardown while executing copy_from_user.
- [High] Lack of rate limiting in validation error paths allows userspace to flood the kernel log and cause a Denial of Service.
- [High] Unsynchronized concurrent writes can trigger premature configuration validation on uninitialized memory.
--

--- Patch [10]: [PATCH v6 10/15] s390/vfio-ap: File ops called to resume the vfio device migration ---
commit 56776518d352cfe0d799d4f95bc7d9c8703337bf
Author: Anthony Krowiak <akrowiak@linux.ibm.com>
Subject: s390/vfio-ap: File ops called to resume the vfio device migration

This commit implements the write callback for the vfio-ap device migration
file stream. It restores the source guest's AP configuration by validating
and hot-plugging it into the destination guest.

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

[ ... ]

> @@ -881,14 +881,20 @@
>  static void report_qinfo_incompatibilities(const char *mdev_name,
>  					   unsigned long apqn,
>  					   struct ap_tapq_hwinfo *src_hwinfo,
>  					   struct ap_tapq_hwinfo *target_hwinfo)
>  {
> +	pr_err("vfio_ap_mdev %s: Migration failed: Source and target queue (%02lx.%04lx) not compatible\n",
> +	       mdev_name, AP_QID_CARD(apqn), AP_QID_QUEUE(apqn));

[Severity: High]
Is it possible for userspace to flood the kernel log here?

If a configuration with a large number of incompatible queues is provided,
verify_ap_configs_are_compatible() continues processing rather than aborting.

Since report_qinfo_incompatibilities() and its helpers issue unconditional
pr_err() calls for each mismatch, could this cause CPU stalls or a denial
of service?

> +
> +	report_facilities_compatibility(mdev_name, apqn, src_hwinfo, target_hwinfo);
> +	report_mode_compatibility(mdev_name, apqn, src_hwinfo, target_hwinfo);

[ ... ]

> @@ -1071,61 +1071,80 @@
>  static ssize_t vfio_ap_resuming_write(struct file *filp, const char __user *buf,
>  				      size_t len, loff_t *pos)
>  {

[ ... ]

> +	/*
> +	 * Snapshot and advance *pos under the lock before dropping it for
> +	 * copy_from_user().  This prevents concurrent write()s on the same
> +	 * stream file from computing the same destination offset and clobbering
> +	 * each other's data or racing to reassign mig_data->resuming_mig_file.
> +	 */
> +	write_pos = *pos;
> +	*pos += len;
> +
> +	mutex_unlock(&matrix_dev->mdevs_lock);
> +
> +	if (copy_from_user((char *)ap_config + write_pos, buf, len)) {
> +		if (new_allocation)
> +			kvfree(ap_config);
> +		return -EIO;
> +	}

[Severity: Critical]
Can a concurrent device teardown cause a use-after-free here?

The mdevs_lock is dropped to perform copy_from_user(). If a concurrent
thread closes the VFIO device, vfio_ap_release_mig_files() will call
kvfree(mig_data->resuming_mig_file.ap_config). Would this result in
copy_from_user() writing into freed kernel memory?

[Severity: Critical]
Could this sequence introduce a double-fetch vulnerability?

The first fetch in allocate_ap_config() reads num_queues to determine the
allocation size. The lock is then dropped, and this second copy_from_user()
overwrites the entire buffer, including the num_queues field.

If userspace modifies num_queues between these fetches, could it inflate
ap_config->num_queues beyond the allocated size, causing out-of-bounds
reads in queues_available() when it loops over source_config->num_queues?

> +
> +	/* Check if we've completed writing the entire configuration */
> +	if (write_pos + len == cfg_sz) {
> +		/*
> +		 * do_post_copy_validation() calls ap_tapq() which is a slow
> +		 * hardware instruction.  Run it before acquiring the update
> +		 * locks to avoid holding guests_lock, kvm->lock, and
> +		 * mdevs_lock across the hardware calls.
> +		 */
> +		ret = do_post_copy_validation(mdev_name, ap_config);

[Severity: High]
Does this completion check safely handle concurrent writes?

If userspace issues asynchronous write() syscalls for different chunks of
the payload, they might execute copy_from_user() concurrently. If the
thread writing the final chunk completes its copy before threads writing
earlier chunks finish theirs, could do_post_copy_validation() be called
on a buffer that still contains uninitialized data?

> +		if (ret < 0) {
> +			if (new_allocation)
> +				kvfree(ap_config);
> +			return ret;
> +		}

[ ... ]

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