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

[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 of mig_data when device FD closes before data FD
- [High] s390/vfio-ap: fortify panic on chunked writes in vfio_ap_resuming_write()
--

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

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

This commit implements the VFIO migration state transitions to and from
VFIO_DEVICE_STATE_STOP in vfio_ap_transition_to_state(), releasing
migration file state when leaving RESUMING, STOP_COPY, or STOP.

> diff --git a/drivers/s390/crypto/vfio_ap_migration.c b/drivers/s390/crypto/vfio_ap_migration.c
> index 47f05e43b9e0e..c5534c6ee3cef 100644
> --- a/drivers/s390/crypto/vfio_ap_migration.c
> +++ b/drivers/s390/crypto/vfio_ap_migration.c

[ ... ]

> @@ -1170,20 +1172,46 @@ vfio_ap_transition_to_state(struct ap_matrix_mdev *matrix_mdev,
>  		return filp;
>  	}
>  
> -	if ((cur_state == VFIO_DEVICE_STATE_RESUMING &&
> -	     new_state == VFIO_DEVICE_STATE_STOP) ||
> -	    (cur_state == VFIO_DEVICE_STATE_STOP_COPY &&
> -	     new_state == VFIO_DEVICE_STATE_STOP)) {
> -		/* TODO */
> -		return ERR_PTR(-EOPNOTSUPP);
> +	/*
> +	 * Terminates the data transfer session of the vfio-ap device state
> +	 * to the destination guest.
> +	 */
> +	if (cur_state == VFIO_DEVICE_STATE_RESUMING &&
> +	    new_state == VFIO_DEVICE_STATE_STOP) {
> +		if (!mig_data->resuming_mig_data_complete)
> +			return ERR_PTR(-EINVAL);
> +
> +		/* Set resuming_mig_data_complete to false for clean reuse */
> +		matrix_mdev->mig_data->resuming_mig_data_complete = false;
> +		vfio_ap_release_resuming_file(matrix_mdev->mig_data);
> +		return NULL;
> +	}
> +
> +	/*
> +	 * Terminates the transfer session of the vfio-ap device state from
> +	 * the source guest.
> +	 */
> +	if (cur_state == VFIO_DEVICE_STATE_STOP_COPY &&
> +	    new_state == VFIO_DEVICE_STATE_STOP) {
> +		vfio_ap_release_stop_copy_file(matrix_mdev->mig_data);
> +		return NULL;
> +	}
> +
> +	/*
> +	 * These states indicate migration has either not been initiated or
> +	 * has completed and the vfio-ap device is operating normally. Since the
> +	 * vfio-ap device does not virtualize a DMA device, there is no internal
> +	 * device state to incorporate into the vfio-ap device on the target.
> +	 */
> +	if (cur_state == VFIO_DEVICE_STATE_STOP &&
> +	    new_state == VFIO_DEVICE_STATE_RUNNING) {
> +		vfio_ap_release_mig_files(matrix_mdev);
> +		return NULL;
>  	}
>  
> -	if ((cur_state == VFIO_DEVICE_STATE_STOP &&
> -	     new_state == VFIO_DEVICE_STATE_RUNNING) ||
> -	    (cur_state == VFIO_DEVICE_STATE_RUNNING &&
> -	     new_state == VFIO_DEVICE_STATE_STOP)) {
> -		/* TODO */
> -		return ERR_PTR(-EOPNOTSUPP);
> +	if (cur_state == VFIO_DEVICE_STATE_RUNNING &&
> +	    new_state == VFIO_DEVICE_STATE_STOP) {
> +		return NULL;
>  	}

[Severity: Critical]
As reported by Sashiko on v5 [1], with the transitions to and from
VFIO_DEVICE_STATE_STOP enabled in vfio_ap_transition_to_state(), userspace can
now enter VFIO_DEVICE_STATE_STOP_COPY or VFIO_DEVICE_STATE_RESUMING and open a
migration stream file descriptor.

When vfio_ap_open_file_stream() creates that file descriptor, it sets
filp->private_data to &matrix_mdev->mig_data->stop_copy_mig_file or
&matrix_mdev->mig_data->resuming_mig_file and pins the registration of
matrix_mdev->vdev via vfio_device_try_get_registration(). However, Both
stop_copy_mig_file and resuming_mig_file are embedded inside struct
vfio_ap_migration_data (matrix_mdev->mig_data) rather than struct
ap_matrix_mdev:

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

When the VFIO device file descriptor is closed while the migration stream file
descriptor is still open (including during process exit, since close_files()
closes file descriptors in ascending order), vfio_ap_mdev_close_device() calls
vfio_ap_release_migration_data(), which destroys the mutexes and frees
matrix_mdev->mig_data:

void vfio_ap_release_migration_data(struct ap_matrix_mdev *matrix_mdev)
{
	lockdep_assert_held(&matrix_dev->mdevs_lock);

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

Doesn't this cause a use-after-free when the open migration file descriptor is
subsequently read in vfio_ap_stop_copy_read(), written in
vfio_ap_resuming_write(), or closed in vfio_ap_release_mig_file(), where
filp->private_data (mig_file) is dereferenced and mig_file->lock is locked or
unlocked after matrix_mdev->mig_data has been freed?

[Severity: High]
In addition, with VFIO_DEVICE_STATE_RESUMING now reachable via
vfio_ap_transition_to_state(), can chunked writes in vfio_ap_resuming_write()
trigger a fortify_panic() when CONFIG_FORTIFY_SOURCE is enabled?

In struct vfio_ap_config, qinfo[] is annotated with __counted_by(num_queues).
While vfio_ap_resuming_init() initializes ap_config->num_queues to
MAX_AP_QUEUES when the stream is opened, vfio_ap_resuming_write() copies
incoming chunks directly into ap_config before checking whether *pos exceeds
expected_sz:

	ap_config = mig_file->ap_config;
	memcpy((char *)ap_config + *pos, kbuf, len);
	*pos += len;

If an earlier write chunk overwrites ap_config->num_queues with a smaller
value (or splits the 4-byte num_queues field across two write calls so that
its upper bytes are zeroed first) and a subsequent write chunk writes past
sizeof(struct vfio_ap_config) + ap_config->num_queues *
sizeof(struct vfio_ap_queue_info), won't fortified memcpy() evaluate the
updated ap_config->num_queues and call fortify_panic() before the bounds check
after memcpy() is reached?

[1] https://lore.kernel.org/r/20260724182949.EA9C51F000E9@smtp.kernel.org

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