Re: [PATCH v5 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 8 potential issue(s) to consider:
- [High] Uninitialized stack variable `guest_matrix` applied to guest configuration.
- [High] Truncated write bypasses bounds check leading to massive out-of-bounds reads.
- [High] Double-fetch of `num_queues` leads to validation bypass (TOCTOU).
- [High] Zero-length write yields ZERO_SIZE_PTR and causes an immediate NULL/invalid pointer dereference.
- [High] Un-serialized writes to the migration stream file lead to heap overflows and memory leaks.
- [High] Use-After-Free of `mig_data` due to concurrent device closure.
- [High] Prolonged execution of slow hardware instructions while holding massive global and KVM lock chains.
- [Medium] High-order memory allocation for `ap_config` breaks live migration reliability.
--

--- Patch [10]: [PATCH v5 10/15] s390/vfio-ap: File ops called to resume the vfio device migration ---
commit e61d7c6d952455d8c5221b566f8371c207901817
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 function to restore the state of
the vfio-ap device during migration. It reads the source guest's AP
configuration from userspace and validates it against the destination host.

> diff --git a/drivers/s390/crypto/vfio_ap_migration.c b/drivers/s390/crypto/vfio_ap_migration.c
> index 4a489a7a05ff1..7a9a8f63a76d3 100644
> --- a/drivers/s390/crypto/vfio_ap_migration.c
> +++ b/drivers/s390/crypto/vfio_ap_migration.c

[ ... ]

> @@ -538,24 +538,36 @@ static ssize_t allocate_ap_config(struct vfio_ap_config **ap_config,
>  				  const char __user *buf, size_t len)
>  {
>  	struct vfio_ap_config tmp_ap_config;
>  	ssize_t config_size;
>  	size_t copy_size;
>  
>  	/*
>  	 * If the length of the data sent exceeds the size of the vfio_ap_config
>  	 * structure, then we will copy enough data from userspace to get the
>  	 * number of queues which we can use to allocate enough space all of
>  	 * the queue information.
>  	 */
>  	copy_size = min(len, sizeof(tmp_ap_config));
>  
>  	if (copy_from_user(&tmp_ap_config, buf, copy_size))
>  		return -EIO;
>  
>  	/*
>  	 * If the length of data sent includes the number of queues
>  	 * in the AP configuration, then calculate its size; otherwise
>  	 * set config_size to the length of data sent.
>  	 */
>  	if (len >= sizeof(struct vfio_ap_config)) {
>  		config_size = calculate_ap_config_size(tmp_ap_config.num_queues);
>  
>  		/* If the calculation returned an error */
>  		if (config_size < 0)
>  			return config_size;
>  	} else {
>  		config_size = len;
>  	}

[Severity: High]
If userspace provides a small length (e.g., 4 bytes), does this bypass the
bounds check? It looks like config_size is set to len, meaning a tiny buffer
is allocated, but later validation loops rely on the unchecked num_queues 
field from this undersized buffer, which could result in a massive 
out-of-bounds heap read.

>  
>  	*ap_config = kzalloc(config_size, GFP_KERNEL_ACCOUNT);

[Severity: Medium]
Since MAX_AP_QUEUES allows up to 65,536 elements, the size calculated by
calculate_ap_config_size() could exceed 1MB. Could this cause legitimate live
migrations to fail if a contiguous order-8 or order-9 physical allocation
cannot be satisfied by kzalloc() due to memory fragmentation? Would it be
safer to use kvzalloc() here to allow falling back to vmalloc?

[Severity: High]
If a zero-length write occurs, len will be 0 and config_size will be set to 0.
Passing 0 to kzalloc() returns ZERO_SIZE_PTR. Because the completion check 
in vfio_ap_resuming_write() (0 + 0 == 0) succeeds immediately, would this
ZERO_SIZE_PTR be passed directly to do_post_copy_validation(), causing an
immediate kernel panic when it attempts to read source_config->num_queues?

[ ... ]

> @@ -1079,22 +1079,30 @@ static void setup_ap_matrix_from_ap_config(struct vfio_ap_config *ap_config,
>  					   struct ap_matrix *guest_matrix)
>  {
>  	struct ap_config_info host_config_info = { 0 };
>  	unsigned long apid, apqi, *guest_adm;
>  	struct vfio_ap_queue_info qinfo;
>  
>  	ap_qci(&host_config_info);
>  	vfio_ap_matrix_init(&host_config_info, guest_matrix);
>  
>  	for (int i = 0; i < ap_config->num_queues; i++) {
>  		qinfo = ap_config->qinfo[i];
>  		apid = AP_QID_CARD(qinfo.apqn);
>  		apqi = AP_QID_QUEUE(qinfo.apqn);
>  
>  		if (!test_bit_inv(apid, guest_matrix->apm))
>  			set_bit_inv(apid, guest_matrix->apm);

[Severity: High]
Does vfio_ap_matrix_init() in drivers/s390/crypto/vfio_ap_ops.c zero out the
apm, aqm, and adm bitmaps in guest_matrix? It appears to only initialize 
scalar max values. Since guest_matrix is declared on the stack in 
vfio_ap_resuming_write() without initialization, using set_bit_inv() here
could leave stack garbage in the bitmaps, potentially granting the destination
guest access to arbitrary unassigned queues.

[ ... ]

> @@ -1113,87 +1121,95 @@ static ssize_t vfio_ap_resuming_write(struct file *filp, const char __user *buf,
>  				      size_t len, loff_t *pos)
>  {
>  	struct vfio_ap_migration_data *mig_data;
>  	struct ap_matrix_mdev *matrix_mdev;
>  	struct vfio_ap_config *ap_config;
>  	struct ap_matrix guest_matrix;
>  	ssize_t ret = 0, cfg_sz;
>  	const char *mdev_name;

[ ... ]

>  	/*
>  	 * We don't want to lock all mdevs while copying data from userspace so
>  	 * we don't block all other mdevs in case the I/O takes a long time.
>  	 * From here on out we don't need this lock because we are not
>  	 * accessing the matrix_mdev until we need to get the update
>  	 * locks to set the new destination guest's AP configuration in which
>  	 * case this lock will be taken then.
>  	 */
>  	mutex_unlock(&matrix_dev->mdevs_lock);
>  
>  	if (copy_from_user((char *)ap_config + *pos, buf, len)) {

[Severity: High]
Since copy_from_user() fetches the entire configuration from the same userspace
buffer pointer a second time, could a concurrent userspace thread modify
num_queues between the initial fetch in allocate_ap_config() and this one?
This could overwrite the validated num_queues value with an arbitrarily large
number, bypassing the sizing logic and leading to an out-of-bounds read later.

>  		ret = -EIO;
>  		kfree(ap_config);
>  
>  		return ret;
>  	}
>  
>  	/* Check if we've completed writing the entire configuration */
>  	if (*pos + len == cfg_sz) {
>  		/* Acquire locks required to update the guest's AP config */
>  		mutex_lock(&ap_attr_mutex);
>  		get_update_locks_for_mdev(matrix_mdev);
>  
>  		/*
>  		 * Verify the device wasn't closed while mdevs_lock was dropped
>  		 * for the copy_from_user above. get_update_locks_for_mdev()
>  		 * reacquires mdevs_lock, so the check is safe here.
>  		 */
>  		if (!matrix_mdev->mig_data) {
>  			release_update_locks_for_mdev(matrix_mdev);
>  			mutex_unlock(&ap_attr_mutex);
>  			kfree(ap_config);
>  			return -ENODEV;
>  		}
>  
>  		ret = do_post_copy_validation(mdev_name, ap_config);

[Severity: High]
The commit message mentions that get_hardware_info_for_queue() was modified 
so it can be executed without holding matrix_dev->mdevs_lock. However, 
do_post_copy_validation() iterates over up to 65,536 elements and calls 
get_hardware_info_for_queue(), which executes the slow ap_tapq() hardware 
instruction. Is it safe to execute this lengthy loop while holding 
ap_attr_mutex, matrix_dev->guests_lock, the KVM lock, and mdevs_lock? Could
this result in severe latency spikes or soft lockups?

[ ... ]

>  	mutex_lock(&matrix_dev->mdevs_lock);
>  	kfree(mig_data->resuming_mig_file.ap_config);
>  	mig_data->resuming_mig_file.ap_config = ap_config;
>  	mig_data->resuming_mig_file.config_sz = cfg_sz;
>  	mutex_unlock(&matrix_dev->mdevs_lock);

[Severity: High]
Could mig_data point to freed memory here? The mig_data pointer was cached at
the start of this function before dropping mdevs_lock. If the device was 
closed concurrently while the lock was dropped for copy_from_user(), wouldn't
this code unconditionally dereference the stale mig_data pointer, leading to a
use-after-free?

>  
>  	ret = len;
>  	*pos += len;

[Severity: High]
Because the migration file is opened with stream_open(), the VFS does not
serialize write() calls with f_pos_lock. Given that mdevs_lock is dropped 
during the copy_from_user() call above, could concurrent writes see the same
shared *pos offset? This seems like it would cause multiple threads to copy
data over the same region, and eventually leak the newly allocated ap_config
buffer when they race to reassign mig_data->resuming_mig_file.ap_config.

>  
>  	return ret;
>  }

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