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

On 8/7/26 6:30 PM, [email protected] wrote:
> 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<[email protected]>
> 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.

The magic/version check will be added to the do_post_copy_validation
function

> [ ... ]
>> +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.

The comment is correct — there are two distinct use-after-free windows
in the original code.

Window 1: during copy_from_user()

mdevs_lock was dropped before calling copy_from_user(). A concurrent
vfio_ap_reset_migration_state() could acquire the lock during that
window, call vfio_ap_release_resuming_file() to free and NULL
resuming_mig_file.ap_config, and return — leaving mig_data intact
but with a NULL ap_config. On the !new_allocation path,
set_new_ap_configuration() would then re-acquire the lock, see
cur_ap_config == NULL, store the temporary ap_config pointer into
resuming_mig_file, and return 0 — causing the caller to immediately
kvfree() the pointer that was just stored there.

Window 2: during do_post_copy_processing()

Even if copy_from_user() completes cleanly,
do_post_copy_processing() drops mdevs_lock internally for slow
hardware TAPQ calls. With the original code storing ap_config into
resuming_mig_file before that call, the same concurrent reset could
free the buffer while do_post_copy_validation() is still reading
through the local pointer parameter.

Fix

Hold mdevs_lock across copy_from_user(). The migration
data is small in practice (a handful of queues → a few hundred bytes
for any real guest), and mdevs_lock is a sleeping mutex so holding
it across a potential page fault is legal.

Keep ap_config as a purely local allocation throughout and store
into resuming_mig_file do_post_copy_processing() returns.

>
> [ ... ]
>> +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.

This is not a bug. The bounds check is performed by
validate_resuming_write_parms(), called unconditionally at the top of
vfio_ap_resuming_write() before any of this code is reached. For
continuation writes (!new_allocation), line 584 enforces:

if (*pos + len > resuming_mig_file.config_sz)
     return -EIO;

For the new_allocation path, resuming_mig_file.ap_config is NULL at
that point (the fresh buffer has not yet been stored there), so that
check does not fire. However, an overrun is still impossible:
write_len = len - nbytes_already_copied writes starting at
write_pos = VFIO_AP_CONFIG_BUF_SIZE, and every subsequent write is
bounded by the same config_sz check above. The total bytes written
across all calls cannot exceed cfg_sz.

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

This is not a bug. The copy_from_user error path
calls kvfree(ap_config) unconditionally — the new_allocation flag
is not checked there at all.

The temporary buffer allocated on the !new_allocation path is freed
on this error path exactly the same as the new_allocation buffer.
There is no leak.

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