Re: [PATCH v8 11/15] s390/vfio-ap: File ops called to resume the vfio device migration

[Anthony Krowiak <akrowiak@linux.ibm.com>]

On 10/9/26 7:56 AM, sashiko-bot@kernel.org wrote:
> 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.

Agreed. Holding ap_attr_mutex, guests_lock, kvm->lock and mdevs_lock
across copy_from_user() lets a buffer that blocks indefinitely stall
all AP attribute updates and mdev operations on the host and the
affected guest.

In the next version, vfio_ap_resuming_write() will copy the user data
into a kernel buffer with vmemdup_user() before taking any lock and use
memcpy to copy the contents of the buffer into ap_config instead of
copy_from_user.

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