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

[Anthony Krowiak <akrowiak@linux.ibm.com>]

On 7/27/26 2:12 PM, sashiko-bot@kernel.org wrote:
> 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?

While it is highly unlikely for this to happen since it would require 
that the
system have an inordinate number of incompatible queue devices, the
pr_err calls will be replaced with pr_err_ratelimited calls.

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

The vfio_ap_open_file_stream calls vfio_device_try_get_registration()
(see patch 06/15) which pins the vfio_device reference until
vfio_ap_release_mig_file() runs — and release only runs when the file
descriptor is closed. The file descriptor cannot be closed while a write()
is in progress on it. So matrix_mdev and its mig_data remain valid for
the lifetime of the write call.

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

A fix will be provided for this in the next patch series.

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

The completion check could theoretically fire early if concurrent writes
are issued, but in that case the validation in do_post_copy_validation
would catch any resulting inconsistency and return an error — the worst
outcome is a failed migration, not a security vulnerability or kernel
corruption.

However, a write_in_progress flag will be added to vfio_ap_migration_data
that will be set when the vfio_ap_resuming_write function is called, unless
the flag has already been set, in which case the function will return 
-EBUSY.write_in_progress

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