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

Thank you for your contribution! Sashiko AI review found 1 potential issue(s) to consider:
- [Critical] Dangling pointer dereference and Use-After-Free (UAF) in `vfio_ap_stop_copy_read` due to dropped lock.
--

--- Patch [8]: [PATCH v7 08/15] s390/vfio-ap: Transition device migration state from STOP to RESUMING ---
commit d54f2e0efd43f553196ad099672d0319fbd7c8d9
Author: Anthony Krowiak <[email protected]>

s390/vfio-ap: Transition device migration state from STOP to RESUMING

Transitioning from VFIO_DEVICE_STATE_STOP to VFIO_DEVICE_STATE_RESUMING
starts a process of restoring the device state of the vfio device on the
target system. To prepare for restoring the device state, open a file
stream to receive the guest's AP configuration saved when the device state
on the source system transitioned from VFIO_DEVICE_STATE_STOP to
VFIO_DEVICE_STATE_STOP_COPY.

> diff --git a/drivers/s390/crypto/vfio_ap_migration.c b/drivers/s390/crypto/vfio_ap_migration.c
> index 50781b61f7f18..e2e7ae8515e55 100644
> --- a/drivers/s390/crypto/vfio_ap_migration.c
> +++ b/drivers/s390/crypto/vfio_ap_migration.c
> @@ -451,6 +451,26 @@ 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;
> +}
> +
> +static const struct file_operations vfio_ap_resume_fops = {
> +	.owner = THIS_MODULE,
> +	.write = vfio_ap_resuming_write,
> +	.release = vfio_ap_release_mig_file,
> +};

[Severity: Critical]
Can a concurrent device close or reset cause a use-after-free when reading the
migration state?

While looking at the migration file operations, I noticed a potential regression
in vfio_ap_stop_copy_read() where mdevs_lock is acquired but explicitly dropped
before copy_to_user():

drivers/s390/crypto/vfio_ap_migration.c:vfio_ap_stop_copy_read() {
    ...
    mutex_unlock(&matrix_dev->mdevs_lock);

    if (copy_to_user(buf, (char *)mig_file->ap_config + read_pos, len))
        return -EFAULT;
}

If userspace concurrently issues a VFIO_DEVICE_RESET ioctl or closes the device
FD while the read thread has dropped mdevs_lock, vfio_ap_release_mig_files()
will acquire mdevs_lock and call kvfree() on
mig_data->stop_copy_mig_file.ap_config.

When the read thread resumes, copy_to_user() will read from the freed buffer,
which could leak kernel memory contents to userspace.

Additionally, mdevs_lock is also dropped in vfio_ap_get_config() for a
potentially long-running operation:

drivers/s390/crypto/vfio_ap_migration.c:vfio_ap_get_config() {
    ...
    mutex_unlock(&matrix_dev->mdevs_lock);
    ret = vfio_ap_store_queue_info(mdev_name, ap_configuration);
    mutex_lock(&matrix_dev->mdevs_lock);
    ...
}

If the device is closed and re-opened during this window, mig_data itself can
be freed and reallocated.

However, vfio_ap_stop_copy_read() caches a local pointer before calling
vfio_ap_get_config():

drivers/s390/crypto/vfio_ap_migration.c:vfio_ap_stop_copy_read() {
    ...
    mig_file = &matrix_mdev->mig_data->stop_copy_mig_file;

    if (!mig_file->ap_config) {
        ret = vfio_ap_get_config(matrix_mdev);
    ...
}

This leaves mig_file dangling when vfio_ap_get_config() returns. This could
lead to a use-after-free when subsequently accessing mig_file->config_sz or
mig_file->ap_config.

Are there protections against mig_data or ap_config being freed while
mdevs_lock is dropped during these operations?

[ ... ]

-- 
Sashiko AI review · https://sashiko.dev/#/patchset/[email protected]?part=8