Re: [PATCH v7 21/23] KVM: s390: arm64: Implement vCPU IOCTLs

[sashiko-bot@kernel.org]

Thank you for your contribution! Sashiko AI review found 7 potential issue(s) to consider:
- [Critical] Double vcpu_load() call in kvm_arch_vcpu_ioctl_run causes preempt notifier list corruption.
- [Critical] Unhandled generic vCPU requests cause an infinite loop in check_vcpu_requests.
- [High] Missing vcpu_load() and vcpu_put() around architectural KVM IOCTLs.
- [High] Host-userspace reachable WARN_ONCE for SAE validity intercepts enables Denial of Service.
- [High] KVM_ARM_VCPU_FINALIZE uses incorrect initialization gates and silently ignores feature finalization.
- [High] TOCTOU race in KVM request checking due to host IRQs being enabled before guest entry.
- [Low] The return value of check_vcpu_requests() is dead code and incorrectly handled.
--

commit 3e429b0d42b7560ba0a2cd3dac96eb26a3ffe9cd
Author: Steffen Eiden <seiden@linux.ibm.com>

KVM: s390: arm64: Implement vCPU IOCTLs

This commit implements the required architectural vCPU IOCTLs for arm64
on s390. It adds the main vCPU execution loop, state initialization,
feature finalization, and handling of architectural registers.

> diff --git a/arch/s390/kvm/arm64/arm.c b/arch/s390/kvm/arm64/arm.c
> index 7df0e4e697e80..3caa72f780a52 100644
> --- a/arch/s390/kvm/arm64/arm.c
> +++ b/arch/s390/kvm/arm64/arm.c
[ ... ]
> @@ -223,6 +247,388 @@ static unsigned long system_supported_vcpu_features(void)
>  	return features;
>  }
>  
[ ... ]
> +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);
> +	}
> +
> +	return 1;
> +}

[Severity: Critical]
Does this code handle other generic KVM requests properly?

If a request other than KVM_REQ_VCPU_RESET or KVM_REQ_IRQ_PENDING is made
(such as KVM_REQ_SLEEP), it seems it will remain pending.

This could interact with the run loop in kvm_arch_vcpu_ioctl_run():

kvm_arch_vcpu_ioctl_run() {
    ...
    if (kvm_request_pending(vcpu)) {
        vcpu->mode = OUTSIDE_GUEST_MODE;
        continue;
    }
    ...
}

Because the unhandled request is never cleared, would the vCPU thread become
permanently stuck in an infinite loop without entering the guest?

[ ... ]
> +int kvm_arch_vcpu_ioctl_run(struct kvm_vcpu *vcpu)
> +{
> +	DECLARE_KERNEL_FPU_ONSTACK32(fpu_save);
> +	struct kvm_run *kvm_run = vcpu->run;
> +	int ret;
> +
> +	if (kvm_run->exit_reason == KVM_EXIT_MMIO) {
> +		ret = kvm_handle_mmio_return(vcpu);
> +		if (ret <= 0)
> +			return ret;
> +	}
> +
> +	vcpu_load(vcpu);

[Severity: Critical]
Could this corrupt the per-CPU preempt notifiers hlist?

The generic KVM ioctl handler in virt/kvm/kvm_main.c already calls
vcpu_load() before invoking kvm_arch_vcpu_ioctl_run(). Calling it a second
time here registers the preempt notifier twice, which appears to create a
self-referential cycle in the hlist and could cause a kernel panic during
scheduling.

[ ... ]
> +	do {
> +		if (signal_pending(current)) {
> +			kvm_run->exit_reason = KVM_EXIT_INTR;
> +			ret = -EINTR;
> +			break;
> +		}
> +
> +		if (need_resched())
> +			schedule();
> +
> +		if (ret > 0)
> +			ret = check_vcpu_requests(vcpu);
> +
> +		vcpu->arch.sae_block.icptr = 0;
> +
> +		smp_store_mb(vcpu->mode, IN_GUEST_MODE);
> +
> +		if (kvm_request_pending(vcpu)) {
> +			vcpu->mode = OUTSIDE_GUEST_MODE;
> +			continue;
> +		}
> +
> +		kvm_vcpu_srcu_read_unlock(vcpu);
> +
> +		arm_vcpu_run(vcpu);
> +
> +		vcpu->mode = OUTSIDE_GUEST_MODE;
> +
> +		kvm_vcpu_srcu_read_lock(vcpu);
> +
> +		ret = handle_exit(vcpu);

[Severity: Low]
Is the return value from check_vcpu_requests() intentionally ignored?

The result is assigned to ret, but there is no check to break out of the
loop if it returns an error. The value is then overwritten by the call to
handle_exit(). If check_vcpu_requests() ever returns an error code, it
appears it would be silently discarded.

[Severity: High]
Is there a race condition here when checking requests with host interrupts
enabled?

The vCPU mode transitions to IN_GUEST_MODE and requests are checked before
interrupts are disabled in arm_vcpu_run(). Furthermore, arm_vcpu_run()
re-enables interrupts just before the sae64a instruction:

arm_vcpu_run() {
    ...
    local_irq_disable();
    guest_timing_enter_irqoff();
    guest_state_enter_irqoff();
    local_irq_enable();

    sae_block->icptr = 0;

    sae64a(sae_block);
    ...
}

If an IPI arrives during this window after the request check but before
sae64a executes, would the kick be lost and leave the vCPU in the guest
without processing the request?

[ ... ]
> +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: {
[ ... ]
> +	case KVM_SET_ONE_REG:
> +	case KVM_GET_ONE_REG: {
> +		struct kvm_one_reg reg;
> +
> +		ret = -ENOEXEC;
> +		if (unlikely(!kvm_vcpu_initialized(vcpu)))
> +			break;
> +
> +		ret = -EFAULT;
> +		if (copy_from_user(&reg, argp, sizeof(reg)))
> +			break;
> +
> +		if (kvm_check_request(KVM_REQ_VCPU_RESET, vcpu))
> +			kvm_reset_vcpu(vcpu);
> +
> +		if (ioctl == KVM_SET_ONE_REG)
> +			ret = kvm_arm_set_reg(vcpu, &reg);
> +		else
> +			ret = kvm_arm_get_reg(vcpu, &reg);
> +		break;
> +	}

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

The kvm_arch_vcpu_ioctl() function routes operations like KVM_SET_ONE_REG and
KVM_GET_ONE_REG, but it appears to lack the necessary vcpu_load() and
vcpu_put() bracketing.

Since generic KVM does not hold the vCPU lock when delegating to
kvm_arch_vcpu_ioctl(), could this allow VCPU state to be modified without
proper synchronization?

[ ... ]
> +	case KVM_ARM_VCPU_FINALIZE: {
> +		int what;
> +
> +		if (!kvm_vcpu_initialized(vcpu))
> +			return -ENOEXEC;
> +
> +		if (get_user(what, (const int __user *)argp))
> +			return -EFAULT;
> +
> +		ret = kvm_arm_vcpu_finalize(vcpu, what);
> +		break;
> +	}

[Severity: High]
Does this code use the correct gate to reject finalization calls?

The operation is gated using !kvm_vcpu_initialized(), but it seems it should
be rejected if the guest has already run. Should this be using
vcpu_has_run_once(vcpu) instead?

Additionally, kvm_arm_vcpu_finalize() in arch/s390/kvm/arm64/reset.c
unconditionally returns 0:

kvm_arm_vcpu_finalize() {
    return 0;
}

Could this cause KVM to silently accept any unsupported features requested by
userspace?

> diff --git a/arch/s390/kvm/arm64/handle_exit.c b/arch/s390/kvm/arm64/handle_exit.c
> new file mode 100644
> index 0000000000000..1286a64783e07
> --- /dev/null
> +++ b/arch/s390/kvm/arm64/handle_exit.c
[ ... ]
> +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);
> +		ret = -EINVAL;
> +		break;
> +	case SAE_ICPTR_HOST_ACCESS_EXCEPTION:
> +	case SAE_ICPTR_SYNCHRONOUS_EXCEPTION:
> +		ret = handle_trap_exceptions(vcpu);
> +		break;
> +	default:
> +		WARN_ONCE(true, "SAE: unknown interception reason 0x%02x",
> +			  icptr);
> +		ret = -EINVAL;
> +	}
> +	return ret;
> +}

[Severity: High]
Are these WARN_ONCE calls reachable by unprivileged userspace or guests?

If userspace injects a malformed SAE state or an unknown intercept code, it
could trigger these warnings. Reaching a WARN_ONCE from guest-controlled
or host-userspace-controlled state can lead to a Denial of Service via log
flooding or a kernel panic if panic_on_warn is enabled.

Could these be changed to return an error or use pr_warn_once() instead?

-- 
Sashiko AI review · https://sashiko.dev/#/patchset/20260831144802.834315-1-seiden@linux.ibm.com?part=21