Re: [PATCH v7 08/15] s390/vfio-ap: Transition device migration state from STOP to RESUMING

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

On 8/7/26 6:41 PM, sashiko-bot@kernel.org wrote:
> Thank you for your contribution! Sashiko AI review found 1 potential issue(s) to consider:
> - [Critical] Dangling pointer dereference and Use-After-Free (UAF) in `vfio_ap_stop_copy_read` due to dropped lock.
> --
>
> --- Patch [8]: [PATCH v7 08/15] s390/vfio-ap: Transition device migration state from STOP to RESUMING ---
> commit d54f2e0efd43f553196ad099672d0319fbd7c8d9
> Author: Anthony Krowiak <akrowiak@linux.ibm.com>
>
> s390/vfio-ap: Transition device migration state from STOP to RESUMING
>
> Transitioning from VFIO_DEVICE_STATE_STOP to VFIO_DEVICE_STATE_RESUMING
> starts a process of restoring the device state of the vfio device on the
> target system. To prepare for restoring the device state, open a file
> stream to receive the guest's AP configuration saved when the device state
> on the source system transitioned from VFIO_DEVICE_STATE_STOP to
> VFIO_DEVICE_STATE_STOP_COPY.
>
>> diff --git a/drivers/s390/crypto/vfio_ap_migration.c b/drivers/s390/crypto/vfio_ap_migration.c
>> index 50781b61f7f18..e2e7ae8515e55 100644
>> --- a/drivers/s390/crypto/vfio_ap_migration.c
>> +++ b/drivers/s390/crypto/vfio_ap_migration.c
>> @@ -451,6 +451,26 @@ static struct file *vfio_ap_open_file_stream(struct ap_matrix_mdev *matrix_mdev,
>>   	return filp;
>>   }
>>   
>> +static ssize_t vfio_ap_resuming_write(struct file *filp, const char __user *buf,
>> +				      size_t len, loff_t *pos)
>> +{
>> +	/* TODO */
>> +	return -EOPNOTSUPP;
>> +}
>> +
>> +static const struct file_operations vfio_ap_resume_fops = {
>> +	.owner = THIS_MODULE,
>> +	.write = vfio_ap_resuming_write,
>> +	.release = vfio_ap_release_mig_file,
>> +};
> [Severity: Critical]
> Can a concurrent device close or reset cause a use-after-free when reading the
> migration state?
>
> While looking at the migration file operations, I noticed a potential regression
> in vfio_ap_stop_copy_read() where mdevs_lock is acquired but explicitly dropped
> before copy_to_user():
>
> drivers/s390/crypto/vfio_ap_migration.c:vfio_ap_stop_copy_read() {
>      ...
>      mutex_unlock(&matrix_dev->mdevs_lock);
>
>      if (copy_to_user(buf, (char *)mig_file->ap_config + read_pos, len))
>          return -EFAULT;
> }
>
> If userspace concurrently issues a VFIO_DEVICE_RESET ioctl or closes the device
> FD while the read thread has dropped mdevs_lock, vfio_ap_release_mig_files()
> will acquire mdevs_lock and call kvfree() on
> mig_data->stop_copy_mig_file.ap_config.
>
> When the read thread resumes, copy_to_user() will read from the freed buffer,
> which could leak kernel memory contents to userspace.
>
> Additionally, mdevs_lock is also dropped in vfio_ap_get_config() for a
> potentially long-running operation:
>
> drivers/s390/crypto/vfio_ap_migration.c:vfio_ap_get_config() {
>      ...
>      mutex_unlock(&matrix_dev->mdevs_lock);
>      ret = vfio_ap_store_queue_info(mdev_name, ap_configuration);
>      mutex_lock(&matrix_dev->mdevs_lock);
>      ...
> }
>
> If the device is closed and re-opened during this window, mig_data itself can
> be freed and reallocated.
>
> However, vfio_ap_stop_copy_read() caches a local pointer before calling
> vfio_ap_get_config():
>
> drivers/s390/crypto/vfio_ap_migration.c:vfio_ap_stop_copy_read() {
>      ...
>      mig_file = &matrix_mdev->mig_data->stop_copy_mig_file;
>
>      if (!mig_file->ap_config) {
>          ret = vfio_ap_get_config(matrix_mdev);
>      ...
> }
>
> This leaves mig_file dangling when vfio_ap_get_config() returns. This could
> lead to a use-after-free when subsequently accessing mig_file->config_sz or
> mig_file->ap_config.
>
> Are there protections against mig_data or ap_config being freed while
> mdevs_lock is dropped during these operations?

This is a legitimate observation.
The fix will be to hold mdevs_lock for the full duration of
vfio_ap_stop_copy_read(), including across copy_to_user() and the
vfio_ap_store_queue_info() call in vfio_ap_get_config(). This
serializes against vfio_ap_release_stop_copy_file() (called from
both vfio_ap_reset_migration_state() and vfio_ap_release_migration_data()),
eliminating both race windows.

>
> [ ... ]
>