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

[Anthony Krowiak <akrowiak@linux.ibm.com>]

On 11/2/26 11:18 AM, sashiko-bot@kernel.org wrote:
> 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?

Confirmed. This is the same mig_data lifetime bug described in the 
review for
03/14.

As you noted, vfio_device_try_get_registration() protects the struct 
vfio_device
and matrix_mdev against unregistration, but did not protect mig_data 
from being
freed when vfio_ap_mdev_close_device() was invoked on device FD close.

In v12, this will be resolved this by tying the lifetime of mig_data
directly to the vfio_device lifetime

>