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

[sashiko-bot@kernel.org]

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 <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 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?

[ ... ]

> @@ -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?

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