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

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

On 11/2/26 12:23 PM, 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 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.

All concerns expressed below have been handled in response to review 
comments
in several other patches. The fixes will be in the forthcoming v12 
series. See
my responses below for details.

>
>> 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?

In v12:
The lifetime of mig_data is tied to the vfio_device registration
rather than the device FD open/close cycle. mig_data is deallocated only in
vfio_ap_mdev_release_dev() after all open migration stream FDs have dropped
their registration references. On device close, vfio_ap_mdev_close_device()
resets the active migration files (clearing filp) via 
vfio_ap_reset_migration_state()
rather than freeing mig_data. Any subsequent or concurrent access on an 
open
migration stream safely encounters valid memory and returns -EINVAL/-ENXIO.

>
> [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

In v12:
The receive buffer in struct vfio_ap_migration_file is tracked as
a raw void * allocation (VFIO_AP_CONFIG_MAX_SIZE). Incoming write chunks
are copied directly into this raw buffer, ensuring fortified memcpy() 
evaluates
bounds against the full buffer allocation rather than the dynamic 
__counted_by
bounds of qinfo[]. Validation of num_queues, expected_sz, and the full
configuration is performed cleanly in kernel code once the complete header
has arrived.

> after memcpy() is reached?
>
> [1] https://lore.kernel.org/r/20260724182949.EA9C51F000E9@smtp.kernel.org
>