Re: [PATCH v3 02/15] s390/vfio-ap: Data structures for facilitating vfio device migration

[sashiko-bot@kernel.org]

Thank you for your contribution! Sashiko AI review found 3 potential issue(s) to consider:
- [Critical] Out-of-bounds read in `do_post_copy_validation` due to redundant and unsynchronized size fields (`num_queues` and `config_sz`) during fragmented migration stream writes.
- [Low] Use of the architecture-dependent type `size_t` in a binary structure intended for serialization.
- [Low] Severe mismatch between kernel-doc comments and actual structure fields in `vfio_ap_migration_data` and `vfio_ap_config`.
--

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

s390/vfio-ap: Data structures for facilitating vfio device migration

This commit introduces the foundational data structures needed to facilitate
state transitions during vfio device migration for the s390 vfio-ap driver.
It adds structures for migration data, queue information, and AP configuration.

> diff --git a/drivers/s390/crypto/vfio_ap_migration.c b/drivers/s390/crypto/vfio_ap_migration.c
> new file mode 100644
> index 0000000000000..239168c4b7fff
> --- /dev/null
> +++ b/drivers/s390/crypto/vfio_ap_migration.c
> @@ -0,0 +1,56 @@

[ ... ]

> +/**
> + * vfio_ap_migration_data - the data needed to migrate a guest with pass-through
> + *			    access to AP devices
> + *
> + * @mig_state:		the current migration state
> + * @resuming_migf:	the object used to resume the target guest
> + * @saving_migf:	the object used to save the state of the source guest
> + */
> +struct vfio_ap_migration_data {
> +	enum vfio_device_mig_state	mig_state;
> +
> +	struct {
> +		struct file *filp;
> +		struct vfio_ap_config	*ap_config;
> +		size_t			config_sz;
> +	} resuming_mig_state;
> +
> +	struct file			*stop_copy_mig_file;
> +};

[Severity: Low]
Are the kernel-doc fields for vfio_ap_migration_data out of sync with the
implementation? The comments document resuming_migf and saving_migf, but the
actual fields are resuming_mig_state and stop_copy_mig_file.

[ ... ]

> +/**
> + * vfio_ap_config - the guest's AP configuration
> + *
> + * @num_queues:	the number of queues passed through to the guest
> + * @reserved: padding to ensure proper alignment of qinfo array
> + * @qinfo:	an array of vfio_ap_queue_info objects, each specifying the
> + *		queue information for a queue passed through to the guest
> + */
> +struct vfio_ap_config {
> +	u32				num_queues;
> +	u8				reserved[4];
> +	size_t				config_sz;

[Severity: Low]
Does the kernel-doc block for vfio_ap_config need an update to include the
config_sz field?

[Severity: Low]
Could the use of size_t here cause consistency issues? Since size_t has
variable width depending on the architecture, using it in a binary structure
intended for serialization over file descriptors or the network is generally
avoided in favor of fixed-width types.

[Severity: Critical]
Could having both num_queues and config_sz in this struct lead to an
out-of-bounds read later during migration?

If a fragmented migration stream write manipulates config_sz to be a smaller
value than expected, the validation checks in reallocate_ap_config() could be
bypassed:

reallocate_ap_config() {
    cur_cfg_sz = ap_config->config_sz;
    num_queues = ap_config->num_queues;
    if (cur_cfg_sz >= sizeof(struct vfio_ap_config)) {
        ...
    } else {
        new_cfg_sz = cur_cfg_sz + len;
    }
}

This could cause an out-of-bounds read in do_post_copy_validation() when
looping num_queues times over the qinfo array:

queues_available_on_target_system() {
    for (int i = 0; i < source_config->num_queues; i++) {
        apqn = source_config->qinfo[i].apqn;
...
}

Is it possible to synchronize these fields or derive the size directly from
num_queues to prevent a mismatch?

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