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

On 7/27/26 2:14 PM, [email protected] wrote:
> Thank you for your contribution! Sashiko AI review found 2 potential issue(s) to consider:
> - [Critical] Use-After-Free in `vfio_ap_stop_copy_read` leading to kernel memory leak to userspace.
> - [Low] The `.release` callback logic for cleaning up the resuming and stop-copy file states is completely unreachable dead code.
> --
>
> --- Patch [8]: [PATCH v6 08/15] s390/vfio-ap: Transition device migration state from STOP to RESUMING ---
> commit 798f82863e85b4d37fd3594dc855fe7caaaaf7f5
> Author: Anthony Krowiak <[email protected]>
>
> 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 e4bc67b1eb842..c7fecad0b6769 100644
>> --- a/drivers/s390/crypto/vfio_ap_migration.c
>> +++ b/drivers/s390/crypto/vfio_ap_migration.c
> [ ... ]
>
>> @@ -99,6 +108,8 @@ static int vfio_ap_release_mig_file(struct inode *file_inode, struct file *filp)
>>   
>>   	if (filp == matrix_mdev->mig_data->stop_copy_mig_file.filp)
>>   		vfio_ap_release_stop_copy_file(matrix_mdev->mig_data);
>> +	else if (filp == matrix_mdev->mig_data->resuming_mig_file.filp)
>> +		vfio_ap_release_resuming_file(matrix_mdev->mig_data);
> [Severity: Low]
> Is this conditional branch actually reachable?
>
> During vfio_ap_open_file_stream(), the driver takes an internal reference to
> the file via get_file(). This means the VFS .release callback is only invoked
> after the driver explicitly drops its reference by calling fput().
>
> If fput() is called in vfio_ap_release_mig_files(), which first sets
> mig_data->resuming_mig_file.filp = NULL, won't filp inside mig_data always
> be NULL by the time this .release callback executes?
>
> Consequently, will this check always evaluate to false, leaving
> vfio_ap_release_resuming_file() as unused dead code?

This is correct; those branches are unreachable. The root cause is
that vfio_ap_release_mig_files() calls fput() on the stored filp and then
immediately sets it to NULL, before the deferred .release callback can
fire. Since fput() always defers __fput() via task_work or a workqueue
(it never calls .release synchronously while a mutex is held), the .release
callback always arrives after the pointer is already NULL — making both
else if branches dead code.

The design will be reworked.

>
> [ ... ]
>
>> @@ -400,6 +411,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]
> Could the migration file stream logic be vulnerable to a use-after-free when
> the device file descriptor is concurrently closed?
>
> Looking at the related vfio_ap_stop_copy_read() stream implementation, it
> drops mdevs_lock before calling 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 the device file descriptor is concurrently closed, vfio_ap_mdev_release()
> could invoke vfio_ap_release_migration_data(), which acquires mdevs_lock and
> frees the mig_data structure.
>
> When vfio_ap_stop_copy_read() resumes, will it dereference the freed mig_data
> structure via mig_file->ap_config and leak kernel memory to userspace?

The use-after-free described is not present. The safety relies on two
invariants:

1. matrix_mdev (and therefore mig_data) cannot be freed while the migration
    FD is open. vfio_ap_open_file_stream() calls
    vfio_device_try_get_registration() which pins matrix_mdev until
    vfio_ap_release_mig_file() drops it. Device teardown
    (vfio_ap_mdev_close_device()) can free mig_data independently, but
    matrix_mdev itself remains valid.

2. mig_file->ap_config is allocated once in vfio_ap_get_config() and freed
    only in vfio_ap_release_stop_copy_file(), which requires mdevs_lock.
    Since vfio_ap_stop_copy_read() advances *pos and snapshots read_pos
    before dropping the lock, any concurrent path that acquires the lock and
    frees ap_config can only do so after the position has already been
    committed — and copy_to_user() only touches the buffer contents, not the
    pointer itself.

>