Re: [PATCH v7 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] Heap buffer overflow in vfio_ap_resuming_write when processing the initial chunk of migration data.
- [High] Memory leak of the dynamically allocated temporary ap_config buffer in vfio_ap_resuming_write on copy_from_user failure.
- [High] Use-After-Free condition in set_new_ap_configuration due to a TOCTOU race when the migration session is concurrently replaced.
- [Medium] Missing validation of migration magic and version fields when the entire configuration payload is submitted in a single write.
--

--- Patch [10]: [PATCH v7 10/15] s390/vfio-ap: File ops called to resume the vfio device migration ---
commit 1f7d34b59709567819d8ff6eb6bf255fe0360e1b
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 file stream created to
restore the vfio-ap device state during the RESUMING phase of migration.
It parses chunks of user data into a vfio_ap_config structure and sets
up the new device configuration.

> diff --git a/drivers/s390/crypto/vfio_ap_migration.c b/drivers/s390/crypto/vfio_ap_migration.c
> index e2e7ae8515e55..4dd7373c3d9d9 100644
> --- a/drivers/s390/crypto/vfio_ap_migration.c
> +++ b/drivers/s390/crypto/vfio_ap_migration.c
[ ... ]
> @@ -451,11 +539,916 @@ static struct file *vfio_ap_open_file_stream(struct ap_matrix_mdev *matrix_mdev,
>  	return filp;
>  }
>  
> +static int validate_resuming_write_parms(struct file *filp,
> +					 size_t len, loff_t *pos)
> +{
[ ... ]
> +	/*
> +	 * If the vfio_ap_config object has been allocated, then the 'config_sz'
> +	 * field indicates the total size allocated. If the write operation
> +	 * will exceed the allocation, return an I/O error.
> +	 */
> +	if (resuming_mig_file.ap_config) {
> +		if (resuming_mig_file.ap_config->magic != VFIO_AP_MIG_MAGIC ||
> +		    resuming_mig_file.ap_config->version != VFIO_AP_MIG_VERSION)
> +			return -EINVAL;
> +
> +		if (*pos + len > resuming_mig_file.config_sz)
> +			return -EIO;
> +	}
> +
> +	return 0;
> +}

[Severity: Medium]
Does this miss validating the magic and version fields if userspace writes
the entire configuration in a single chunk? The validation here is skipped
if the configuration buffer is not yet allocated. If all data arrives at
once, it appears to be allocated and processed in do_post_copy_processing()
without the magic or version ever being verified against the expected
values.

[ ... ]
> +static int set_new_ap_configuration(struct ap_matrix_mdev *matrix_mdev,
> +				    struct vfio_ap_config *ap_config,
> +				    size_t cfg_sz)
> +{
> +	struct vfio_ap_config *cur_ap_config;
> +
> +	mutex_lock(&matrix_dev->mdevs_lock);
> +	/*
> +	 * Re-read mig_data under the lock; the device could have been closed
> +	 * concurrently while the lock was dropped for copy_from_user().
> +	 */
> +	if (!matrix_mdev->mig_data) {
> +		mutex_unlock(&matrix_dev->mdevs_lock);
> +		return -ENODEV;
> +	}
> +
> +	cur_ap_config = matrix_mdev->mig_data->resuming_mig_file.ap_config;
> +	if (cur_ap_config)
> +		memcpy(cur_ap_config, ap_config, cfg_sz);
> +	else
> +		matrix_mdev->mig_data->resuming_mig_file.ap_config = ap_config;

[Severity: High]
Can a concurrent state change cause a use-after-free here? The mdevs_lock is
dropped prior to the copy_from_user() call in vfio_ap_resuming_write(). If
the migration state is cleared during that window, the original ap_config
pointer would be freed and cur_ap_config will be NULL upon reacquiring the
lock. This path then assigns the temporary ap_config to resuming_mig_file.
However, if the write was a continuation (!new_allocation), the temporary
ap_config is unconditionally freed at the end of vfio_ap_resuming_write(),
which would leave a dangling pointer in the active migration session.

[ ... ]
> +static ssize_t vfio_ap_resuming_write(struct file *filp, const char __user *buf,
> +				      size_t len, loff_t *pos)
> +{
[ ... ]
> +	/*
> +	 * If ap_config is a new allocation, then the contents of the
> +	 * 'magic', 'version' and 'num_queues' fields will already have
> +	 * been copied in; so the write_pos must be set to the location
> +	 * following the 'num_queues' field and the length to be written must be
> +	 * adjusted accordingly.
> +	 */
> +	if (new_allocation) {
> +		size_t nbytes_already_copied = VFIO_AP_CONFIG_BUF_SIZE - *pos;
> +
> +		write_pos = VFIO_AP_CONFIG_BUF_SIZE;
> +		write_len = len - nbytes_already_copied;
> +		buf += nbytes_already_copied;
> +	} else {
> +		write_pos = *pos;
> +		write_len = len;
> +	}
> +
> +	*pos += len;
> +
> +	mutex_unlock(&matrix_dev->mdevs_lock);
> +
> +	if (copy_from_user((char *)ap_config + write_pos, buf, write_len)) {

[Severity: Critical]
Is there a missing bounds check on the user-supplied len parameter here?
When allocating a new ap_config, the driver calculates cfg_sz based on the
num_queues field. However, the calculation for write_len uses the unmodified
user input len, which is not verified against cfg_sz. If a userspace process
provides a len that exceeds the dynamically calculated cfg_sz, it seems this
copy_from_user() will write past the allocated heap buffer bounds.

[Severity: High]
Does this leak the dynamically allocated temporary ap_config buffer when a
continuation write fails? During a continuation write (!new_allocation),
the ap_config buffer is freshly allocated via kvzalloc(). If the subsequent
copy_from_user() fails, this error path explicitly skips the kvfree()
because new_allocation is false, leaving the temporary buffer permanently
leaked.

> +		if (new_allocation)
> +			kvfree(ap_config);
> +		ret = -EFAULT;
> +		goto out_clear_write_in_progress;
> +	}
> +
> +	/* Check if we've completed writing the entire configuration */
> +	if (write_pos + write_len == cfg_sz) {
> +		ret = do_post_copy_processing(matrix_mdev, ap_config);
> +
> +		if (ret) {
> +			kvfree(ap_config);
> +			goto out_clear_write_in_progress;
> +		}
> +	}

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