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

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

On 8/7/26 6:30 PM, sashiko-bot@kernel.org wrote:

All of the observations in this email are resolved with a simplification 
of the
vfio_ap_resuming_write() function that is forthcoming in the next version
of the patch series.

> 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;
>> +		}
>> +	}