Re: [PATCH v4 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 2 potential issue(s) to consider:
- [High] Migration file streams do not decouple their lifecycle from the vfio_device, allowing `matrix_mdev` to be accessed after being freed.
- [Medium] Concurrent reads on the migration state file cause a data race on `filp->f_pos`.
--

--- Patch [8]: [PATCH v4 08/15] s390/vfio-ap: Transition device migration state from STOP to RESUMING ---
commit df7f5fdcda2503aec3804ab2b04e4b736cd6f0a1
Author: Anthony Krowiak <akrowiak@linux.ibm.com>

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 5ef7127b697a5..e917cc995d352 100644
> --- a/drivers/s390/crypto/vfio_ap_migration.c
> +++ b/drivers/s390/crypto/vfio_ap_migration.c

[ ... ]

> @@ -74,6 +85,8 @@ static int vfio_ap_release_mig_file(struct inode *file_inode, struct file *filp)
>  
>  	if (filp == matrix_mdev->mig_data->stop_copy_mig_file)
>  		vfio_ap_release_stop_copy_file(matrix_mdev->mig_data);
> +	else if (filp == matrix_mdev->mig_data->resuming_mig_state.filp)
> +		vfio_ap_release_resuming_file(matrix_mdev->mig_data);
>  	else
>  		return -ENOENT;

[Severity: High]
Does this expose the matrix_mdev lifecycle to the file descriptor?

Migration file streams do not seem to decouple their lifecycle from the
vfio_device. The migration state file descriptor is created using
anon_inode_getfile() with matrix_mdev set as its private_data.

If a user closes the main device file descriptor but keeps the migration
file descriptor open, and the administrator unbinds the mdev device, the core
VFIO layer unregisters and frees matrix_mdev. 

When the user later closes the migration file descriptor,
vfio_ap_release_mig_file() dereferences filp->private_data to access the
already freed matrix_mdev:

static int vfio_ap_release_mig_file(...)
{
    struct ap_matrix_mdev *matrix_mdev = filp->private_data;
    ...
    mutex_lock(&matrix_dev->mdevs_lock);
    if (filp == matrix_mdev->mig_data->stop_copy_mig_file)

Could this lead to a use-after-free of matrix_mdev when attempting to clean
up the migration state?

[ ... ]

> @@ -311,6 +324,32 @@ static struct file *vfio_ap_open_file_stream(struct ap_matrix_mdev *matrix_mdev,
>  	return filp;
>  }

[Severity: Medium]
Is it possible for concurrent operations on these migration state files to
cause a data race on filp->f_pos?

vfio_ap_open_file_stream() initializes the file with stream_open(), which sets
FMODE_STREAM and clears FMODE_ATOMIC_POS. In the VFS layer, this skips
acquiring f_pos_lock for stream files.

Looking at vfio_ap_stop_copy_read() which uses these stream files:

    pos = &filp->f_pos;
    ...
    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 matrix_dev->mdevs_lock is released before copy_to_user() and before
*pos += len is executed, wouldn't concurrent read() calls read from the same
*pos offset, perform copy_to_user() with duplicate data, and concurrently
update *pos?

[ ... ]

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