Re: [PATCH v7 09/15] s390/vfio-ap: Add method to set a new guest AP configuration

Thank you for your contribution! Sashiko AI review found 1 potential issue(s) to consider:
- [Critical] Migration data and buffers can be freed by close_device() while vfio_ap_stop_copy_read() is reading them, resulting in multiple Use-After-Free vulnerabilities.
--

--- Patch [9]: [PATCH v7 09/15] s390/vfio-ap: Add method to set a new guest AP configuration ---
commit 6fc20ee0bf1a514a8193425ef0cd6ee0f1b7529a
Author: Anthony Krowiak <[email protected]>

s390/vfio-ap: Add method to set a new guest AP configuration

Adds a new vfio_ap_set_new_config function to set a guest's AP
configuration. This is needed in order to set the state of the mdev when
it is migrated from a remote host system during the RESUMING phase.

> diff --git a/drivers/s390/crypto/vfio_ap_ops.c b/drivers/s390/crypto/vfio_ap_ops.c
> index d05372b50d2fc..0f33f51891536 100644
> --- a/drivers/s390/crypto/vfio_ap_ops.c
> +++ b/drivers/s390/crypto/vfio_ap_ops.c

[ ... ]

[Severity: Critical]
Can this result in a Use-After-Free during concurrent device close and read
operations?

Looking at vfio_ap_stop_copy_read() in drivers/s390/crypto/vfio_ap_migration.c,
if a concurrent close_device occurs while reading migration data:

    if (*pos >= mig_file->config_sz) {

mig_file caches a pointer into mig_data, which may have been freed during the
lock drop in vfio_ap_get_config() if the device was concurrently closed and
reopened.

Furthermore, mdevs_lock is deliberately dropped before this line:

    if (copy_to_user(buf, (char *)mig_file->ap_config + read_pos, len))

allowing a concurrent close_device to free both mig_data (which mig_file points
into) and ap_config.

This happens because vfio_ap_mdev_close_device() in
drivers/s390/crypto/vfio_ap_ops.c unconditionally frees migration structures
even when migration FDs remain open:

    vfio_ap_release_migration_data(matrix_mdev);

A malicious userspace program could use userfaultfd to pause execution of
copy_to_user() in vfio_ap_stop_copy_read(). Concurrently, it could close the
device FD, invoking vfio_ap_mdev_close_device() and freeing the migration
structures. Once the userfaultfd is resolved, copy_to_user() would read from the
freed and potentially poisoned memory, copying it to userspace.

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