Re: [PATCH v3 06/15] s390/vfio-ap: Transition guest migration state from STOP to STOP_COPY

[sashiko-bot@kernel.org]

Thank you for your contribution! Sashiko AI review found 2 potential issue(s) to consider:
- [High] The migration file descriptor does not hold a reference to `matrix_mdev` nor uses an independent context structure, leading to a Use-After-Free when the migration file is closed after the device is removed.
- [High] `vfio_ap_release_mig_file()` accesses and modifies `matrix_mdev->mig_data` without holding `matrix_dev->mdevs_lock`, leading to a TOCTOU race and Use-After-Free.
--

commit 24121dd0a897a029f9bc2979eb21b1bf31847440
Author: Anthony Krowiak <akrowiak@linux.ibm.com>

s390/vfio-ap: Transition guest migration state from STOP to STOP_COPY

This patch implements the transition to the STOP_COPY migration state for
vfio-ap devices. It sets up the migration infrastructure by creating a file
stream used to read the guest's AP configuration. The file operations are
introduced with a stub read implementation.

> diff --git a/drivers/s390/crypto/vfio_ap_migration.c b/drivers/s390/crypto/vfio_ap_migration.c
> index 8274e7b65eea4..76e06abe5a9e9 100644
> --- a/drivers/s390/crypto/vfio_ap_migration.c
> +++ b/drivers/s390/crypto/vfio_ap_migration.c
[ ... ]
> @@ -57,6 +58,57 @@ struct vfio_ap_config {
>  	struct vfio_ap_queue_info	qinfo[] __counted_by(num_queues);
>  };
>  
[ ... ]
> +static int vfio_ap_release_mig_file(struct inode *file_inode, struct file *filp)
> +{
> +	struct ap_matrix_mdev *matrix_mdev = filp->private_data;
> +

[Severity: High]
Does this code race with device closure?

If userspace concurrently closes the main VFIO device FD and the migration
FD, one thread could process the main FD closure, acquire the mdevs_lock,
and free matrix_mdev->mig_data.

If vfio_ap_release_mig_file() is called by the other thread and accesses
matrix_mdev->mig_data without acquiring matrix_dev->mdevs_lock, could it
dereference a freed mig_data pointer?

> +	if (!matrix_mdev || !matrix_mdev->mig_data)
> +		return -ENODEV;
> +
> +	if (filp == matrix_mdev->mig_data->stop_copy_mig_file)
> +		vfio_ap_release_stop_copy_file(matrix_mdev->mig_data);
> +	else
> +		return -ENOENT;
> +
> +	return 0;
> +}
> +
> +static const struct file_operations vfio_ap_stop_copy_fops = {
> +	.owner = THIS_MODULE,
> +	.read = vfio_ap_stop_copy_read,
> +	.compat_ioctl = compat_ptr_ioctl,
> +	.release = vfio_ap_release_mig_file,
> +};
> +
> +static struct file *vfio_ap_open_file_stream(struct ap_matrix_mdev *matrix_mdev,
> +					     const struct file_operations *fops,
> +					     int flags)
> +{
> +	struct file *filp;
> +
> +	lockdep_assert_held(&matrix_dev->mdevs_lock);
> +
> +	filp = anon_inode_getfile("vfio_ap_mig_file", fops, matrix_mdev, flags);

[Severity: High]
Can this lead to a use-after-free of matrix_mdev?

Because the migration file descriptor does not hold a reference to
matrix_mdev, what happens if userspace closes the main VFIO device FD
while keeping the migration FD open, and the administrator removes the
mediated device?

The VFIO core might wait for the main device FD to close and then free the
matrix_mdev object. When userspace eventually closes the migration FD,
vfio_ap_release_mig_file() will dereference filp->private_data which
points to the freed matrix_mdev memory.

> +	if (!IS_ERR(filp))
> +		stream_open(filp->f_inode, filp);
> +
> +	return filp;
> +}

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