Re: [PATCH v6 31/33] KVM: s390: arm64: Implement vCPU IOCTLs

Thank you for your contribution! Sashiko AI review found 6 potential issue(s) to consider:
- [Critical] kvm_arch_vcpu_ioctl_run() fails to acquire the kvm->srcu read lock, leading to a Use-After-Free when accessing memslots during guest aborts.
- [High] Guest-reachable WARN_ONCE on SAE validity intercept enables a host DoS.
- [High] Calling local_irq_enable() immediately after guest_enter_irqoff() violates strict RCU extended quiescent state (EQS) rules.
- [High] check_vcpu_requests() fails to handle critical KVM requests like KVM_REQ_TLB_FLUSH and KVM_REQ_VM_DEAD.
- [High] Missing IN_GUEST_MODE coordination in the vCPU run loop creates a race condition that can delay or drop signals and KVM requests.
- [High] Missing vcpu_load() and vcpu_put() around KVM IOCTLs that modify architectural state in kvm_arch_vcpu_ioctl.
--

--- Patch [31]: [PATCH v6 31/33] KVM: s390: arm64: Implement vCPU IOCTLs ---
commit e43ea7af3ba7491497b01c2a0860a8b946aad58d
Author: Steffen Eiden <[email protected]>

KVM: s390: arm64: Implement vCPU IOCTLs

This commit implements the required vCPU IOCTLs for the s390 arm64 KVM
port. It provides the core structures for register access, lifecycle
management, and the main vCPU execution loop.

> diff --git a/arch/s390/kvm/arm64/arm.c b/arch/s390/kvm/arm64/arm.c
> --- a/arch/s390/kvm/arm64/arm.c
> +++ b/arch/s390/kvm/arm64/arm.c

[ ... ]

> @@ -284,15 +309,15 @@
> +static int check_vcpu_requests(struct kvm_vcpu *vcpu)
> +{
> +	if (kvm_request_pending(vcpu)) {
> +		if (kvm_check_request(KVM_REQ_VCPU_RESET, vcpu))
> +			kvm_reset_vcpu(vcpu);
> +		/*
> +		 * Clear IRQ_PENDING requests that were made to guarantee
> +		 * that a VCPU sees new virtual interrupts.
> +		 */
> +		kvm_check_request(KVM_REQ_IRQ_PENDING, vcpu);
> +	}

[Severity: High]
Does this function properly handle all critical KVM requests?

It appears to drop requests like KVM_REQ_TLB_FLUSH and KVM_REQ_VM_DEAD by
not explicitly handling or clearing them. Can this bypass necessary TLB
invalidations and dead VM checks?

> +
> +	return 1;
> +}

[ ... ]

> @@ -390,13 +415,13 @@
> +static void arm_vcpu_run(struct kvm_vcpu *vcpu)
> +{
> +	struct kvm_sae_block *sae_block = &vcpu->arch.sae_block;
> +
> +	adjust_pc(vcpu);
> +
> +	local_irq_disable();
> +	guest_enter_irqoff();
> +	local_irq_enable();

[Severity: High]
Does this sequence violate RCU extended quiescent state rules?

Calling local_irq_enable() immediately after guest_enter_irqoff() allows
host interrupts to fire while RCU believes the CPU is idle.

> +
> +	sae_block->icptr = 0;
> +
> +	sae64a(sae_block);

[ ... ]

> @@ -436,15 +461,15 @@
> +	ret = 1;
> +	do {
> +		if (signal_pending(current)) {
> +			kvm_run->exit_reason = KVM_EXIT_INTR;
> +			ret = -EINTR;
> +			continue;
> +		}
> +
> +		if (need_resched())
> +			schedule();
> +
> +		if (ret > 0)
> +			ret = check_vcpu_requests(vcpu);
> +
> +		vcpu->arch.sae_block.icptr = 0;
> +
> +		arm_vcpu_run(vcpu);

[Severity: High]
Could this loop race with concurrent IPI kicks?

It executes arm_vcpu_run() without coordinating via vcpu->mode = IN_GUEST_MODE.
A concurrent request kick via kvm_vcpu_kick() might see OUTSIDE_GUEST_MODE
and skip sending the IPI.

> +
> +		ret = handle_exit(vcpu);

[Severity: Critical]
Does handle_exit() require SRCU protection here?

The loop calls handle_exit() which routes synchronous exceptions to the guest
abort handler. If the abort handler translates guest addresses and accesses
active memslots without holding kvm->srcu, could a concurrent userspace
operation like KVM_SET_USER_MEMORY_REGION trigger a use-after-free?

> +
> +	} while (ret > 0);

[ ... ]

> @@ -515,15 +540,15 @@
> +long kvm_arch_vcpu_ioctl(struct file *filp, unsigned int ioctl, unsigned long arg)
> +{
> +	struct kvm_vcpu *vcpu = filp->private_data;
> +	void __user *argp = (void __user *)arg;
> +	struct kvm_device_attr attr;
> +	int ret;
> +
> +	switch (ioctl) {
> +	case KVM_ARM_VCPU_INIT: {
> +		struct kvm_vcpu_init init;

[Severity: High]
Are vcpu_load() and vcpu_put() required around these ioctls?

Operations like KVM_SET_ONE_REG modify architectural state but do not acquire
the vcpu_load locks, potentially bypassing preempt notifiers and physical-CPU
hardware state tracking hooks.

> +
> +		ret = -EFAULT;
> +		if (copy_from_user(&init, argp, sizeof(init)))
> +			break;

[ ... ]

> diff --git a/arch/s390/kvm/arm64/handle_exit.c b/arch/s390/kvm/arm64/handle_exit.c
> --- a/arch/s390/kvm/arm64/handle_exit.c
> +++ b/arch/s390/kvm/arm64/handle_exit.c

[ ... ]

> @@ -66,13 +78,13 @@
> +int handle_exit(struct kvm_vcpu *vcpu)
> +{
> +	u8 icptr = vcpu->arch.sae_block.icptr;
> +	int ret = 1;
> +
> +	switch (icptr) {
> +	case SAE_ICPTR_SPURIOUS:
> +		break;
> +	case SAE_ICPTR_VALIDITY:
> +		WARN_ONCE(true, "SAE: validity intercept. vir: 0x%04x",
> +			  vcpu->arch.sae_block.vir);

[Severity: High]
Is it safe to use a guest-reachable WARN_ONCE here?

An unprivileged userspace process configuring KVM could set malformed guest
state via KVM_SET_ONE_REG, and the hardware would reject this during
execution, returning a validity intercept. Should this return an error to
userspace instead of triggering a host warning?

> +		ret = -EINVAL;
> +		break;

[ ... ]

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