Re: [PATCH v8 11/15] 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 1 potential issue(s) to consider:
- [High] locking: DoS via userfaultfd stalling under global mutexes in vfio_ap_resuming_write()
--

commit 0439535c7d2f8e78d7863822666908892163a421
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 file stream used to
restore the vfio-ap device state on a destination system during the
RESUMING migration phase. It copies the source guest's AP configuration
from userspace, validates its compatibility with the destination host,
and applies it if compatible.

> diff --git a/drivers/s390/crypto/vfio_ap_migration.c b/drivers/s390/crypto/vfio_ap_migration.c
> index 7b9851b79ec5f..caae95b47a9aa 100644
> --- a/drivers/s390/crypto/vfio_ap_migration.c
> +++ b/drivers/s390/crypto/vfio_ap_migration.c
[ ... ]
> @@ -448,11 +504,644 @@ 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)
> +{
[ ... ]
> +	/*
> +	 * Hold ap_attr_mutex for the full duration of the write, just as
> +	 * (un)assign_adapter_store() and (un)assign_domain_store() do.  This
> +	 * prevents the AP bus apmask/aqmask attributes from being changed
> +	 * while the guest's AP configuration is being updated.
> +	 *
> +	 * get_update_locks_for_mdev() then acquires guests_lock, kvm->lock,
> +	 * and mdevs_lock in the correct order. This serialises
> +	 * the entire operation against concurrent AP attribute writers
> +	 * while the guest's AP configuration is being changed.
> +	 *
> +	 * Holding these locks for the full duration of a potentially chunked
> +	 * write could in theory affect performance, but this is acceptable for
> +	 * several reasons: migration is an infrequent operation; real-world
> +	 * guest AP configurations are small (each CEX8 card has 2 adapters and
> +	 * up to 16 domains, and the 85 cards in a system are shared across all
> +	 * LPARs and guests); and each chunk write completes quickly.
> +	 */
> +	matrix_mdev = filp->private_data;
> +	mutex_lock(&ap_attr_mutex);
> +	get_update_locks_for_mdev(matrix_mdev);
> +	pos = &filp->f_pos;
> +
> +	ret = validate_resuming_write_parms(filp, len);
> +	if (ret)
> +		goto out_unlock;
> +
> +	ap_config = matrix_mdev->mig_data->resuming_mig_file.ap_config;
> +
> +	if (copy_from_user((char *)ap_config + *pos, buf, len)) {

[Severity: High]
Does this code allow a malicious userspace process to stall the kernel
indefinitely using userfaultfd?

Holding global subsystem mutexes (ap_attr_mutex, matrix_dev->guests_lock,
matrix_dev->mdevs_lock) and the VM's kvm->lock during copy_from_user()
allows a compromised userspace process (such as a QEMU instance) to stall
execution by mapping the buffer with userfaultfd and never resolving the
page fault. 

This would freeze all AP bus attribute changes and vfio_ap mdev lifecycle
operations globally across the host, and lock up the affected KVM instance.
Even without userfaultfd, normal page faults from swap or network
filesystems could cause unpredictable and severe global latency spikes.

> +		ret = -EFAULT;
> +		goto out_unlock;
> +	}
> +
> +	*pos += len;
[ ... ]

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