Re: [PATCH v4 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 1 potential issue(s) to consider:
- [Critical] The migration file descriptor's lifecycle is not decoupled from the VFIO device, and its release callback accesses device structures locklessly. This causes a Use-After-Free and TOCTOU races if the migration file outlives the VFIO device or if it is closed concurrently with device teardown.
--

--- Patch [6]: [PATCH v4 06/15] s390/vfio-ap: Transition guest migration state from STOP to STOP_COPY ---
commit 2960f99fd69c88ca78cea015d8ad75148e325f63
Author: Anthony Krowiak <akrowiak@linux.ibm.com>

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

This commit introduces the transition to the STOP_COPY migration state for
vfio-ap devices. It creates a migration file descriptor using an anonymous
inode to stream the guest's AP configuration, providing a stub read
implementation and a release callback.

> diff --git a/drivers/s390/crypto/vfio_ap_migration.c b/drivers/s390/crypto/vfio_ap_migration.c
> --- a/drivers/s390/crypto/vfio_ap_migration.c
> +++ b/drivers/s390/crypto/vfio_ap_migration.c

[ ... ]

> +static int vfio_ap_release_mig_file(struct inode *file_inode, struct file *filp)
> +{
> +	struct ap_matrix_mdev *matrix_mdev = filp->private_data;
> +
> +	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);

[Severity: Critical]
Is there a risk of a use-after-free here if the main VFIO device is closed
or unbound concurrently while the migration file descriptor is still open?

Since this release callback accesses matrix_mdev and its mig_data locklessly,
could matrix_mdev have already been freed during device teardown if userspace
closes the device but keeps the migration file descriptor open?

> +	else
> +		return -ENOENT;
> +
> +	return 0;
> +}

[ ... ]

> +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: Critical]
Does this code safely couple the migration file descriptor's lifecycle to the
VFIO device?

Because matrix_mdev is passed as private_data without taking a registration
reference on the VFIO device or wrapping it in an independent lifecycle
object, it seems the VFIO core will not wait for this migration fd to close
before tearing down the device.

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

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