Re: [PATCH v3 08/15] s390/vfio-ap: Transition device migration state from STOP to RESUMING

[sashiko-bot@kernel.org]

Thank you for your contribution! Sashiko AI review found 3 potential issue(s) to consider:

Pre-existing issues:
- [High] The migration file descriptor lacks a reference to the vfio_device, and its release handler lacks locking, leading to critical Use-After-Free and NULL pointer dereferences.
- [High] Concurrent read operations in vfio_ap_stop_copy_read bypass VFS locks and race on filp->f_pos, corrupting the migration stream.
- [High] Missing initialization of config_sz in vfio_ap_get_config causes a zero value to be sent to userspace, breaking the resuming phase protocol.
--

commit fb4283ff8475958b596a7e1a1f9bf2aba56d90d8
Author: Anthony Krowiak <akrowiak@linux.ibm.com>
Subject: s390/vfio-ap: Transition device migration state from STOP to RESUMING

This commit introduces the transition from VFIO_DEVICE_STATE_STOP to
VFIO_DEVICE_STATE_RESUMING. It prepares for restoring the device state by
opening a file stream to receive the guest's AP configuration. It adds a stub
implementation for the write operation.

> diff --git a/drivers/s390/crypto/vfio_ap_migration.c b/drivers/s390/crypto/vfio_ap_migration.c
> index 5ef7127b697a5..e917cc995d352 100644
> --- a/drivers/s390/crypto/vfio_ap_migration.c
> +++ b/drivers/s390/crypto/vfio_ap_migration.c
> @@ -65,6 +65,17 @@ vfio_ap_release_stop_copy_file(struct vfio_ap_migration_data *mig_data)
>  		mig_data->stop_copy_mig_file = NULL;
>  }
>  
> +static void
> +vfio_ap_release_resuming_file(struct vfio_ap_migration_data *mig_data)
> +{
> +	if (mig_data->resuming_mig_state.filp)
> +		mig_data->resuming_mig_state.filp = NULL;
> +
> +	kfree(mig_data->resuming_mig_state.ap_config);
> +	mig_data->resuming_mig_state.ap_config = NULL;
> +	mig_data->resuming_mig_state.config_sz = 0;
> +}
> +
>  static int vfio_ap_release_mig_file(struct inode *file_inode, struct file *filp)
>  {
>  	struct ap_matrix_mdev *matrix_mdev = filp->private_data;

[Severity: High]
This is a pre-existing issue, but does vfio_ap_release_mig_file() safely
access matrix_mdev and mig_data here?

It appears the migration file descriptor is created in
vfio_ap_open_file_stream() without taking a reference to the vfio_device via
vfio_device_get(). Because of this, the file descriptor can outlive the
device structure.

If userspace closes the main device file descriptor and unbinds the mdev,
matrix_mdev is freed. When the migration file descriptor is subsequently
closed, vfio_ap_release_mig_file() accesses the freed matrix_mdev.

Also, since this release handler doesn't hold matrix_dev->mdevs_lock, can it
race with vfio_ap_release_migration_data() freeing mig_data, leading to a
use-after-free or NULL pointer dereference when accessing
matrix_mdev->mig_data?

[ ... ]

> @@ -311,6 +324,32 @@ 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;
> +}

[Severity: High]
This is a pre-existing issue, but while implementing the resumption phase,
will the data coming from vfio_ap_get_config() be properly formed?

In vfio_ap_get_config():

ap_configuration = kzalloc(*ap_config_size, GFP_KERNEL_ACCOUNT);
if (!ap_configuration)
    return -ENOMEM;

ap_configuration->num_queues = num_queues;

The config_sz field is never initialized, so a zero value is copied to
userspace. During this write operation, if data is handled in chunks and
uses config_sz to calculate the new allocation size, could it read 0, shrink
the buffer with krealloc() to an undersized amount, and cause an
out-of-bounds write during copy_from_user()?

> +
> +static const struct file_operations vfio_ap_resume_fops = {
> +	.owner = THIS_MODULE,
> +	.write = vfio_ap_resuming_write,
> +	.release = vfio_ap_release_mig_file,
> +};

[Severity: High]
This isn't a bug introduced by this patch, but considering these new file
operations, is the f_pos handling safe from data races under concurrent I/O?

Looking at the existing vfio_ap_stop_copy_read(), the file's internal
position is updated entirely outside the protection of
matrix_dev->mdevs_lock:

vfio_ap_stop_copy_read()
   mutex_lock(&matrix_dev->mdevs_lock);
   ...
   mutex_unlock(&matrix_dev->mdevs_lock);

   if (copy_to_user(buf, (char *)ap_config + *pos, len)) {
       kfree(ap_config);
       return -EFAULT;
   }

   kfree(ap_config);
   *pos += len;

Since stream_open() disables the standard VFS f_pos_lock serialization,
can concurrent read() calls race here, sample the same offset, transmit
duplicate data blocks, and double-increment the position pointer?

[ ... ]

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