Re: [PATCH 2/2] KVM: arm64: Add VGIC v3 batch register access implementation

[sashiko-bot@kernel.org]

Thank you for your contribution! Sashiko AI review found 4 potential issue(s) to consider:
- [Critical] kvm: arm64: vgic: allow userspace to bypass VGIC locks via KVM_DEV_ARM_VGIC_ATTR_FLAG_LOCKED
- [High] kvm: arm64: vgic: user memory access under VM locks in batch access enables deadlock
- [Medium] kvm: arm64: vgic: un-ratelimited pr_err in batch access allows userspace log flooding
- [Medium] kvm: arm64: vgic: batch access fails to return first error or copy results to userspace
--

commit 4bb3fc1e279eb19b2c3ca453e47c56bcab743a11
Author: Yize Wang <wangyize7@huawei.com>

KVM: arm64: Add VGIC v3 batch register access implementation

This commit adds vgic_v3_batch_access() to allow userspace to read or write
multiple VGIC registers in a single system call. It introduces a new flag
KVM_DEV_ARM_VGIC_ATTR_FLAG_LOCKED to prevent repeated locking and unlocking
during these batch accesses.

> diff --git a/arch/arm64/kvm/vgic/vgic-kvm-device.c b/arch/arm64/kvm/vgic/vgic-kvm-device.c
> --- a/arch/arm64/kvm/vgic/vgic-kvm-device.c
> +++ b/arch/arm64/kvm/vgic/vgic-kvm-device.c
> @@ -566,14 +566,21 @@ static int vgic_v3_attr_regs_access(struct kvm_device *dev,
>  			return -EFAULT;
>  	}
>  
> -	mutex_lock(&dev->kvm->lock);
> +	/* If the caller has not indicated that it already holds the necessary locks
> +	 * (e.g., via KVM_DEV_ARM_VGIC_ATTR_FLAG_LOCKED), we must acquire the VM lock
> +	 * and pause all VCPUs to ensure consistent access to VGIC state. This prevents
> +	 * race conditions with concurrent interrupt injection or VCPU execution.
> +	 */
> +	if (attr->flags != KVM_DEV_ARM_VGIC_ATTR_FLAG_LOCKED) {
> +		mutex_lock(&dev->kvm->lock);

[Severity: Critical]
Can userspace bypass the VGIC data structure locks by setting this flag
directly?

If a userspace process invokes KVM_SET_DEVICE_ATTR or KVM_GET_DEVICE_ATTR
with attr->flags explicitly set to KVM_DEV_ARM_VGIC_ATTR_FLAG_LOCKED,
vgic_v3_attr_regs_access() will skip acquiring kvm->lock,
kvm->arch.config_lock, and all VCPU mutexes.

Could this allow concurrent modifications to internal VGIC state, leading
to race conditions or use-after-free regressions?

[ ... ]

> +/* vgic_v3_batch_access - Batch access to VGIC registers
> + *
> + * @dev: The VGIC device
> + * @attr: The device attribute containing the batch entries
> + * @is_write: True for write, false for read
> + *
> + * This function allows userspace to read or write multiple VGIC registers
> + * in a single system call, reducing overhead during live migration.
> + *
> + * Returns: 0 on success, or the first error encountered.
> + * Individual entry errors are stored in entries[i].error.
> + */
> +static int vgic_v3_batch_access(struct kvm_device *dev,
> +				struct kvm_device_attr *attr, bool is_write)
> +{
[ ... ]
> +	mutex_lock(&dev->kvm->lock);
> +	if (kvm_trylock_all_vcpus(dev->kvm)) {
> +		mutex_unlock(&dev->kvm->lock);
> +		kvfree(entries);
> +		return -EBUSY;
> +	}
> +	mutex_lock(&dev->kvm->arch.config_lock);
> +
> +	/* Process each entry in the batch */
> +	for (i = 0; i < count; i++) {
> +		int err;
> +
> +		sub_attr.group = entries[i].group;
> +		sub_attr.attr = entries[i].attr;
> +		sub_attr.addr = (unsigned long)uentries +
> +			i * sizeof(struct kvm_dev_arm_vgic_batch_entry) +
> +			offsetof(struct kvm_dev_arm_vgic_batch_entry, val);
> +
> +		/* Set flag to indicate that locks are already held.
> +		 * This prevents vgic_v3_attr_regs_access from acquiring locks again,
> +		 * avoiding deadlock and overhead.
> +		 */
> +		sub_attr.flags = KVM_DEV_ARM_VGIC_ATTR_FLAG_LOCKED;
> +
> +		err = vgic_v3_attr_regs_access(dev, &sub_attr, is_write);

[Severity: High]
Does executing a user memory access while holding VM locks introduce a
deadlock regression?

Here sub_attr.addr is assigned a userspace address (uentries), which is
then passed into vgic_v3_attr_regs_access(). Inside
vgic_v3_attr_regs_access(), this address is accessed using put_user()
or get_user():

    if (!ret && uaccess && !is_write) {
        u32 __user *uaddr = (u32 __user *)(unsigned long)attr->addr;
        ret = put_user(val, uaddr);
    }

Since the batch caller holds kvm->lock, kvm->arch.config_lock, and all
vcpu->mutexes, could an untrusted userspace process point uentries to
userfaultfd-managed memory and intentionally stall the page fault? This
would keep all global VM and VCPU locks held indefinitely, hanging the VM.

> +
> +		if (err) {
> +			pr_err("group %d attr %lld get err %d\n",
> +				sub_attr.group, sub_attr.attr, err);

[Severity: Medium]
Is there a risk of userspace flooding the host kernel log buffer here?

This is an unconditional pr_err() driven by user-supplied inputs. Since
there is no rate limiting and the loop does not abort on error, a batch of
up to 4096 invalid VGIC attribute entries could spam dmesg and degrade
host performance.

> +			ret = err;
> +		}
> +	}
> +
> +	mutex_unlock(&dev->kvm->arch.config_lock);
> +	kvm_unlock_all_vcpus(dev->kvm);
> +	mutex_unlock(&dev->kvm->lock);
> +
> +	kvfree(entries);
> +	return ret;
> +}

[Severity: Medium]
Does this code fail to return the first error or copy the results back to
userspace?

The function header indicates it should return the first error and store
individual errors in entries[i].error. However, the loop overwrites ret
with the most recent error instead of returning immediately.

Furthermore, the entries array is freed here without being copied back to
user memory, meaning userspace has no way to determine which register
access actually failed.

-- 
Sashiko AI review · https://sashiko.dev/#/patchset/20260918081930.4014735-1-wangyize7@huawei.com?part=2