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

[sashiko-bot@kernel.org]

Thank you for your contribution! Sashiko AI review found 1 potential issue(s) to consider:
- [Critical] s390/vfio-ap: use-after-free of mig_file in vfio_ap_release_mig_file()
--

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

s390/vfio-ap: Transition device migration state from STOP to RESUMING

This commit implements the transition from VFIO_DEVICE_STATE_STOP to
VFIO_DEVICE_STATE_RESUMING in vfio_ap_transition_to_state() by pre-allocating
the receive buffer for the guest's AP configuration and opening a write-only
anonymous inode file stream for the resuming migration data.

> diff --git a/drivers/s390/crypto/vfio_ap_migration.c b/drivers/s390/crypto/vfio_ap_migration.c
> index b64ce092a41b1..526d83e8590d9 100644
> --- a/drivers/s390/crypto/vfio_ap_migration.c
> +++ b/drivers/s390/crypto/vfio_ap_migration.c

[ ... ]

> @@ -456,6 +466,67 @@ static struct file *vfio_ap_open_file_stream(struct vfio_ap_migration_file *mig_
>  	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,
> +};

[ ... ]

> +static struct file *vfio_ap_resuming_init(struct vfio_ap_migration_file *mig_file)
> +{
> +	struct vfio_ap_config *ap_config;
> +	struct file *filp;
> +
> +	lockdep_assert_held(&matrix_dev->mdevs_lock);
> +
> +	ap_config = kvzalloc(VFIO_AP_CONFIG_MAX_SIZE, GFP_KERNEL_ACCOUNT);
> +	if (!ap_config)
> +		return ERR_PTR(-ENOMEM);
> +
> +	ap_config->num_queues = MAX_AP_QUEUES;
> +
> +	filp = vfio_ap_open_file_stream(mig_file, &vfio_ap_resume_fops, O_WRONLY);
> +
> +	if (IS_ERR(filp)) {
> +		kvfree(ap_config);
> +		return filp;
> +	}
> +
> +	mig_file->ap_config = ap_config;
> +	mig_file->config_sz = VFIO_AP_CONFIG_MAX_SIZE;
> +	mig_file->filp = filp;
> +
> +	return filp;
> +}
> +
>  static struct file *
>  vfio_ap_transition_to_state(struct ap_matrix_mdev *matrix_mdev,
>  			    enum vfio_device_mig_state new_state)
> @@ -488,10 +559,19 @@ vfio_ap_transition_to_state(struct ap_matrix_mdev *matrix_mdev,
>  		return filp;
>  	}
>  
> +	/*
> +	 * Begins the process of restoring the vfio device state by creating and
> +	 * returning a streaming data_fd to be used to read in the internal
> +	 * state of the vfio-ap device on the destination host.
> +	 */
>  	if (cur_state == VFIO_DEVICE_STATE_STOP &&
>  	    new_state == VFIO_DEVICE_STATE_RESUMING) {
> -		/* TODO */
> -		return ERR_PTR(-EOPNOTSUPP);
> +		struct file *filp = vfio_ap_resuming_init(&mig_data->resuming_mig_file);
> +
> +		if (IS_ERR(filp))
> +			return ERR_CAST(filp);
> +
> +		return filp;
>  	}

[Severity: Critical]
Can closing the VFIO device file descriptor before the migration stream file
descriptor trigger a use-after-free in vfio_ap_release_mig_file()?

In vfio_ap_transition_to_state(), vfio_ap_resuming_init() passes
&mig_data->resuming_mig_file (and the STOP_COPY path similarly passes
&mig_data->stop_copy_mig_file) to vfio_ap_open_file_stream(), which stores
mig_file in filp->private_data and pins the vfio_device registration:

drivers/s390/crypto/vfio_ap_migration.c:vfio_ap_open_file_stream() {
	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);
}

Pinning the registration keeps matrix_mdev alive, but resuming_mig_file and
stop_copy_mig_file are embedded in struct vfio_ap_migration_data
(matrix_mdev->mig_data), which is dynamically allocated when the VFIO device
file descriptor is opened and freed when it is closed via
vfio_ap_mdev_close_device() -> vfio_ap_release_migration_data():

drivers/s390/crypto/vfio_ap_migration.c:vfio_ap_release_migration_data() {
	if (!matrix_mdev->mig_data)
		return;

	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;
}

If the VFIO device file descriptor is closed while the migration stream file
descriptor is still open (for example, if a userspace VMM exits or crashes
during migration and close_files() closes descriptors in ascending order),
matrix_mdev->mig_data is freed while filp->private_data still points into it.

When the migration stream file descriptor is subsequently accessed or closed,
vfio_ap_release_mig_file() dereferences mig_file->matrix_mdev from the freed
mig_data allocation:

drivers/s390/crypto/vfio_ap_migration.c:vfio_ap_release_mig_file() {
	struct vfio_ap_migration_file *mig_file = filp->private_data;
	...
	vfio_device_put_registration(&mig_file->matrix_mdev->vdev);
	return 0;
}

And any concurrent or subsequent read/write on that migration file descriptor
will also lock the destroyed and freed mig_file->lock.

Should mig_data (or the vfio_ap_migration_file structures) be embedded in
struct ap_matrix_mdev or otherwise tied to the lifetime of the open migration
file descriptors?

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