Re: [PATCH v11 09/14] 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: use-after-free of mig_file in vfio_ap_resuming_write()
- [High] s390/vfio-ap: fortify panic in vfio_ap_resuming_write() on chunked writes
--

commit 05b49746b964ad1916518e2c783de5c6220f1f14
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 migration file stream used
during the RESUMING phase to receive and validate the source guest's AP
configuration on the destination host.

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

[ ... ]

> @@ -466,11 +511,562 @@ static struct file *vfio_ap_open_file_stream(struct vfio_ap_migration_file *mig_
>  	return filp;
>  }
>  
> +static int validate_resuming_write_parms(struct vfio_ap_migration_file *mig_file,
> +					 struct file *filp, size_t len)
> +{
> +	loff_t total_len;
> +
> +	lockdep_assert_held(&mig_file->lock);
> +	lockdep_assert_held(&matrix_dev->mdevs_lock);
> +
> +	if (!len || filp->f_pos < 0)
> +		return -EINVAL;
> +
> +	if (!mig_file->matrix_mdev->mig_data)
> +		return -ENODEV;

[ ... ]

>  static ssize_t vfio_ap_resuming_write(struct file *filp, const char __user *buf,
>  				      size_t len, loff_t *pos)
>  {
> -	/* TODO */
> -	return -EOPNOTSUPP;
> +	struct vfio_ap_migration_file *mig_file;
> +	struct vfio_ap_config *ap_config;
> +	ssize_t ret;
> +	void *kbuf;

[ ... ]

> +	mig_file = filp->private_data;
> +	mutex_lock(&mig_file->lock);
> +	mutex_lock(&ap_attr_mutex);
> +	mutex_lock(&matrix_dev->mdevs_lock);
> +	pos = &filp->f_pos;
> +
> +	ret = validate_resuming_write_parms(mig_file, filp, len);
> +	if (ret)
> +		goto out_unlock;

[Severity: High]
Can this trigger a use-after-free on mig_file if the VFIO device file
descriptor is closed while the migration file descriptor remains open?

In vfio_ap_resuming_init() -> vfio_ap_open_file_stream(), filp->private_data
is set to &mig_data->resuming_mig_file, which is embedded inside
matrix_mdev->mig_data:

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

While vfio_ap_open_file_stream() calls vfio_device_try_get_registration() to
keep matrix_mdev alive until vfio_ap_release_mig_file() runs, closing the
VFIO device file descriptor invokes:

vfio_ap_mdev_close_device()
  vfio_ap_release_migration_data() {
	...
	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;
  }

Although validate_resuming_write_parms() checks
!mig_file->matrix_mdev->mig_data under matrix_dev->mdevs_lock, mig_file
itself lives inside the freed matrix_mdev->mig_data object. Calling
vfio_ap_resuming_write() after or concurrently with
vfio_ap_mdev_close_device() will lock and unlock the destroyed
mig_file->lock and dereference mig_file->matrix_mdev in freed memory.

> +
> +	ap_config = mig_file->ap_config;
> +	memcpy((char *)ap_config + *pos, kbuf, len);
> +	*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;
> +		}

[Severity: High]
As reported by Sashiko on v9 [1], can the memcpy() in
vfio_ap_resuming_write() trigger a fortify_panic() on chunked writes when
CONFIG_FORTIFY_SOURCE and CONFIG_CC_HAS_COUNTED_BY are enabled?

In struct vfio_ap_config, qinfo[] is annotated 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);
};

Although vfio_ap_resuming_init() initializes ap_config->num_queues to
MAX_AP_QUEUES, fortified memcpy((char *)ap_config + *pos, kbuf, len)
evaluates __struct_size((char *)ap_config + *pos) using the current value of
ap_config->num_queues in memory before the expected_sz check runs:

1. Once an earlier chunk writes the header with num_queues < MAX_AP_QUEUES,
   the dynamic object size of *ap_config shrinks to expected_sz. If a
   subsequent write() supplies more bytes than expected_sz - *pos (while
   staying within VFIO_AP_CONFIG_MAX_SIZE), memcpy() executes before the
   *pos > (loff_t)expected_sz check and calls fortify_panic() instead of
   returning -EINVAL.

2. On big-endian s390, MAX_AP_QUEUES (0x00010000) has bytes 0x00, 0x01,
   0x00, 0x00 at offsets 8..11. If an initial chunk of 10 or 11 bytes writes
   the upper two bytes of a valid num_queues < 65536 (0x00, 0x00),
   ap_config->num_queues in memory becomes 0. If the next chunk writes past
   the header into qinfo[], fortified memcpy() evaluates __struct_size()
   with ap_config->num_queues == 0 before copying the remaining bytes of
   num_queues, triggering fortify_panic() on valid migration data.

[1] https://lore.kernel.org/r/sashiko-outbox-170099@kernel.org

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