Re: [PATCH v11 06/14] 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:
- [Critical] s390/vfio-ap: use-after-free in vfio_ap_release_mig_file() after device close
- [Critical] s390/vfio-ap: use-after-free of mig_file in vfio_ap_release_mig_file()
--

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

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

This commit implements the migration state transition from
VFIO_DEVICE_STATE_STOP to VFIO_DEVICE_STATE_STOP_COPY by opening an
anonymous file stream for reading the guest's AP configuration.

> diff --git a/drivers/s390/crypto/vfio_ap_migration.c b/drivers/s390/crypto/vfio_ap_migration.c
> index 8e4e04e039658..48028b12b26f9 100644
> --- a/drivers/s390/crypto/vfio_ap_migration.c
> +++ b/drivers/s390/crypto/vfio_ap_migration.c
[ ... ]
> @@ -143,6 +144,62 @@ static void vfio_ap_release_resuming_file(struct vfio_ap_migration_data *mig_dat
>  	mig_data->resuming_mig_data_complete = false;
>  }
>  
> +static ssize_t
> +vfio_ap_stop_copy_read(struct file *filp, char __user *buf, size_t len,
> +		       loff_t *pos)
> +{
> +	/* TODO */
> +	return -EOPNOTSUPP;
> +}
> +
> +static int vfio_ap_release_mig_file(struct inode *file_inode, struct file *filp)
> +{
> +	struct vfio_ap_migration_file *mig_file = filp->private_data;
> +
> +	/*
> +	 * The mig_file (and by extension mig_file->matrix_mdev) is tied to
> +	 * migration FD lifetime (see vfio_ap_open_file_stream() function) to
> +	 * guarantee that the matrix_mdev remains valid for every read/write
> +	 * operation. Let's drop it here since the migration FD is being
> +	 * released.
> +	 */
> +	vfio_device_put_registration(&mig_file->matrix_mdev->vdev);
> +	return 0;
> +}
> +
> +static const struct file_operations vfio_ap_stop_copy_fops = {
> +	.owner = THIS_MODULE,
> +	.read = vfio_ap_stop_copy_read,
> +	.release = vfio_ap_release_mig_file,
> +};
> +
> +static struct file *vfio_ap_open_file_stream(struct vfio_ap_migration_file *mig_file,
> +					     const struct file_operations *fops,
> +					     int flags)
> +{
> +	struct file *filp;
> +
> +	lockdep_assert_held(&matrix_dev->mdevs_lock);
> +
> +	/*
> +	 * Pin the vfio_device registration so that matrix_mdev cannot be freed
> +	 * while the migration FD is still open. The matching put is in
> +	 * vfio_ap_release_mig_file().
> +	 */
> +	if (!vfio_device_try_get_registration(&mig_file->matrix_mdev->vdev))
> +		return ERR_PTR(-ENODEV);
> +
> +	filp = anon_inode_getfile("vfio_ap_mig_file", fops, mig_file, flags);
> +	if (IS_ERR(filp)) {
> +		vfio_device_put_registration(&mig_file->matrix_mdev->vdev);
> +		return filp;
> +	}
> +
> +	stream_open(filp->f_inode, filp);
> +
> +	return filp;
> +}
> +
>  static struct file *
>  vfio_ap_transition_to_state(struct ap_matrix_mdev *matrix_mdev,
>  			    enum vfio_device_mig_state new_state)
> @@ -156,10 +213,23 @@ vfio_ap_transition_to_state(struct ap_matrix_mdev *matrix_mdev,
>  	dev_dbg(matrix_mdev->vdev.dev, "%s: %d -> %d\n", __func__, cur_state,
>  		new_state);
>  
> +	/*
> +	 * Begins the process of saving the vfio device state by creating and
> +	 * returning a streaming data_fd to be used to read out the internal
> +	 * state of the vfio-ap device on the source host.
> +	 */
>  	if (cur_state == VFIO_DEVICE_STATE_STOP &&
>  	    new_state == VFIO_DEVICE_STATE_STOP_COPY) {
> -		/* TODO */
> -		return ERR_PTR(-EOPNOTSUPP);
> +		struct vfio_ap_migration_file *mig_file = &mig_data->stop_copy_mig_file;
> +		struct file *filp = vfio_ap_open_file_stream(mig_file,
> +							     &vfio_ap_stop_copy_fops,
> +							     O_RDONLY);
> +		if (IS_ERR(filp))
> +			return ERR_CAST(filp);
> +
> +		mig_file->filp = filp;
> +
> +		return filp;
>  	}

[Severity: Critical]
Can this trigger a use-after-free of mig_file in vfio_ap_release_mig_file()
(and in vfio_ap_stop_copy_read() in the subsequent patch) if the VFIO device
file descriptor is closed while the migration stream file descriptor remains
open?

In vfio_ap_transition_to_state(), mig_file points to
&mig_data->stop_copy_mig_file, which is embedded inside struct
vfio_ap_migration_data:

struct vfio_ap_migration_data {
	enum vfio_device_mig_state	mig_state;
	bool				resuming_mig_data_complete;
	struct vfio_ap_migration_file	resuming_mig_file;
	struct vfio_ap_migration_file	stop_copy_mig_file;
};

vfio_ap_open_file_stream() stores mig_file in filp->private_data and calls
vfio_device_try_get_registration(&mig_file->matrix_mdev->vdev). However,
vfio_device_try_get_registration() only pins struct vfio_device (and thus
matrix_mdev) against unregistration; it does not prevent vfio_df_close()
from invoking vfio_ap_mdev_close_device() when the VFIO device FD is closed
(which also happens during normal process exit when close_files() closes the
lower-numbered VFIO device FD before the higher-numbered migration data FD).

When the VFIO device FD is closed, vfio_ap_mdev_close_device() calls
vfio_ap_release_migration_data(), which destroys stop_copy_mig_file.lock and
frees matrix_mdev->mig_data:

void vfio_ap_release_migration_data(struct ap_matrix_mdev *matrix_mdev)
{
	...
	vfio_ap_release_mig_files(matrix_mdev);
	mutex_destroy(&matrix_mdev->mig_data->stop_copy_mig_file.lock);
	mutex_destroy(&matrix_mdev->mig_data->resuming_mig_file.lock);
	kfree(matrix_mdev->mig_data);
	matrix_mdev->mig_data = NULL;
}

When the migration stream FD is subsequently closed (or read),
vfio_ap_release_mig_file() retrieves mig_file from filp->private_data and
dereferences mig_file->matrix_mdev from the freed mig_data allocation (and
if the VFIO device was reopened in the meantime, a new mig_data is allocated
while the old migration FD still points to the freed mig_data instance).

Should filp->private_data point to matrix_mdev (or should mig_file/mig_data
lifetime be tied to the open migration FDs) so that releasing or reading the
migration FD does not dereference freed memory?

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