Re: [PATCH v10 10/15] s390/vfio-ap: File ops called to resume the vfio device migration

[sashiko-bot@kernel.org]

Thank you for your contribution! Sashiko AI review found 2 potential issue(s) to consider:
- [High] s390/vfio-ap: fortify panic in vfio_ap_resuming_write() due to __counted_by
- [Medium] s390/vfio-ap: corrupt f_pos and completion state in vfio_ap_resuming_write()
--

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

s390/vfio-ap: File ops called to resume the vfio device migration

This commit implements the write callback for the resuming migration file
stream in drivers/s390/crypto/vfio_ap_migration.c, accumulating the source
guest's AP configuration into a pre-allocated vfio_ap_config buffer and
validating compatibility against the destination mdev's shadow_apcb and
hardware capabilities.

> diff --git a/drivers/s390/crypto/vfio_ap_migration.c b/drivers/s390/crypto/vfio_ap_migration.c
> index 9ed657c7302e9..469fa09ec9087 100644
> --- a/drivers/s390/crypto/vfio_ap_migration.c
> +++ b/drivers/s390/crypto/vfio_ap_migration.c

[ ... ]

> @@ -451,11 +506,562 @@ static struct file *vfio_ap_open_file_stream(struct ap_matrix_mdev *matrix_mdev,
>  	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;
> +	struct ap_matrix_mdev *matrix_mdev;
> +	struct vfio_ap_config *ap_config;
> +	ssize_t ret;
> +	void *kbuf;

[ ... ]

> +	matrix_mdev = filp->private_data;
> +	mutex_lock(&ap_attr_mutex);
> +	get_update_locks_for_mdev(matrix_mdev);
> +	pos = &filp->f_pos;
> +
> +	ret = validate_resuming_write_parms(filp, len);
> +	if (ret)
> +		goto out_unlock;
> +
> +	ap_config = matrix_mdev->mig_data->resuming_mig_file.ap_config;
> +
> +	memcpy((char *)ap_config + *pos, kbuf, len);

[Severity: High]
Will this memcpy() in vfio_ap_resuming_write() trigger a fortify_panic() when
built with CONFIG_FORTIFY_SOURCE=y and CONFIG_CC_HAS_COUNTED_BY=y?

In drivers/s390/crypto/vfio_ap_migration.c, struct vfio_ap_config declares its
flexible array member with __counted_by(num_queues):

struct vfio_ap_config {
	u32				magic;
	u32				version;
	u32				num_queues;
	u8				reserved[4];
	u64				adm[DIV_ROUND_UP(AP_DOMAINS, 64)];
	struct vfio_ap_queue_info	qinfo[] __counted_by(num_queues);
};

When vfio_ap_resuming_init() allocates ap_config via kvzalloc(),
ap_config->num_queues is zero-initialized. In vfio_ap_resuming_write(),
fortified memcpy() evaluates __builtin_dynamic_object_size() on
(char *)ap_config + *pos before copying the bytes from kbuf. Because
qinfo[] is annotated with __counted_by(num_queues), the compiler computes the
remaining object size from the current value of ap_config->num_queues:

  sizeof(struct vfio_ap_config) +
  ap_config->num_queues * sizeof(struct vfio_ap_queue_info) - *pos

When userspace writes a configuration containing one or more queues in a
single write() call (or a chunk spanning across num_queues into qinfo[]),
ap_config->num_queues is still 0 when the bounds check runs, so p_size is
at most sizeof(struct vfio_ap_config) while len is larger, causing
fortify_memcpy_chk() to call fortify_panic().

Similarly, even if an earlier chunk already populated ap_config->num_queues,
memcpy() runs before the *pos > (loff_t)expected_sz check below, so a write
that exceeds expected_sz will also trigger fortify_panic() instead of
returning -EINVAL.

> +	*pos += len;
> +
> +	/*
> +	 * Check whether we have received all the data.  We know the total
> +	 * expected size once num_queues has arrived, i.e. once *pos is past
> +	 * the fixed header.  Until then keep accumulating.
> +	 */
> +	if (*pos >= offsetofend(struct vfio_ap_config, num_queues)) {
> +		size_t expected_sz;
> +
> +		if (ap_config->num_queues > MAX_AP_QUEUES) {
> +			ret = -EINVAL;
> +			goto out_unlock;
> +		}
> +
> +		expected_sz = sizeof(struct vfio_ap_config) +
> +			      ap_config->num_queues *
> +			      sizeof(struct vfio_ap_queue_info);
> +
> +		if (*pos > (loff_t)expected_sz) {
> +			ret = -EINVAL;
> +			goto out_unlock;
> +		}
> +
> +		if (*pos == (loff_t)expected_sz) {
> +			ret = do_post_copy_processing(matrix_mdev, ap_config);
> +			matrix_mdev->mig_data->resuming_mig_data_complete = !ret;
> +		}
> +	}

[Severity: Medium]
Can advancing *pos and writing into ap_config before validating num_queues
and expected_sz leave filp->f_pos, ap_config, and resuming_mig_data_complete
in an inconsistent state on error?

In vfio_ap_resuming_write(), memcpy() and *pos += len execute unconditionally
before checking ap_config->num_queues > MAX_AP_QUEUES and
*pos > (loff_t)expected_sz. If either check fails, or if
do_post_copy_processing() fails, vfio_ap_resuming_write() returns a negative
errno while leaving filp->f_pos advanced and the invalid data in ap_config.

In addition, if a valid configuration is written completely such that
*pos == (loff_t)expected_sz and do_post_copy_processing() succeeds,
matrix_mdev->mig_data->resuming_mig_data_complete is set to true. If a
subsequent write arrives with extra trailing bytes before the device
transitions out of VFIO_DEVICE_STATE_RESUMING, vfio_ap_resuming_write()
returns -EINVAL (or -EIO in validate_resuming_write_parms()) without clearing
resuming_mig_data_complete, leaving the stream marked complete after a failed
write.

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