Re: [PATCH v5 29/31] KVM: s390: arm64: Implement vCPU IOCTLs

Thank you for your contribution! Sashiko AI review found 5 potential issue(s) to consider:
- [Critical] VCPU run loop calls schedule() and might_fault() while preemption is disabled by kernel_fpu_begin().
- [High] Guest- or host-userspace-reachable WARN_ONCE triggers allow log spamming and potential kernel panics.
- [High] Calling local_irq_enable() immediately after guest_enter_irqoff() breaks RCU extended quiescent state invariants.
- [High] The vCPU run loop fails to set IN_GUEST_MODE, leading to lost cross-vCPU IPIs.
- [Medium] KVM_ARM_VCPU_FINALIZE silently accepts all feature requests by returning success (0) unconditionally.
--

--- Patch [29]: [PATCH v5 29/31] KVM: s390: arm64: Implement vCPU IOCTLs ---
commit 5a1c5a40e465ee169cdb685d1f98ec6e23195cc2
Author: Steffen Eiden <[email protected]>

KVM: s390: arm64: Implement vCPU IOCTLs

This commit implements all required vCPU IOCTLs for KVM s390 arm64.
It introduces handlers for KVM_RUN, KVM_SET_ONE_REG, and feature
finalization logic.

> 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

[ ... ]

> @@ -385,8 +385,11 @@ static void arm_vcpu_run(struct kvm_vcpu *vcpu)
>  	adjust_pc(vcpu);
>  
>  	local_irq_disable();
>  	guest_enter_irqoff();
>  	local_irq_enable();

[Severity: High]
Does calling local_irq_enable() immediately after guest_enter_irqoff() break
RCU extended quiescent state invariants? guest_enter_irqoff() explicitly
transitions the CPU into an RCU Extended Quiescent State (EQS), instructing
RCU to ignore this CPU. Enabling hardware interrupts here allows them to fire
while the CPU is still marked as being in an EQS. Any interrupt handler that
executes would run with RCU not watching, bypassing read-side protections.

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

[ ... ]

> @@ -420,25 +420,38 @@ int kvm_arch_vcpu_ioctl_run(struct kvm_vcpu *vcpu)
>  	vcpu_load(vcpu);
>  
>  	kernel_fpu_begin(&fpu_save, KERNEL_FPC | KERNEL_VXR);
>  	load_vx_regs((vcpu->arch.ctxt.vregs));
>  
>  	if (!vcpu->wants_to_run) {
>  		ret = -EINTR;
>  		goto out;
>  	}
>  
>  	kvm_sigset_activate(vcpu);
>  
>  	might_fault();
>  
>  	ret = 1;
>  	do {
>  		if (signal_pending(current)) {
>  			kvm_run->exit_reason = KVM_EXIT_INTR;
>  			ret = -EINTR;
>  			continue;
>  		}
>  
>  		if (need_resched())
>  			schedule();

[Severity: Critical]
Does this code call schedule() and might_fault() while preemption is disabled?
kernel_fpu_begin() disables preemption to safely manage hardware FPU state.
Inside this atomic section, the code calls might_fault() and schedule(), which
will trigger a fatal scheduling while atomic BUG or cause silent FPU state
corruption.

>  
>  		if (ret > 0)
>  			ret = check_vcpu_requests(vcpu);
>  
>  		vcpu->arch.sae_block.icptr = 0;
>  
>  		arm_vcpu_run(vcpu);

[Severity: High]
Does this loop need to set vcpu->mode = IN_GUEST_MODE before entering the
guest execution phase? When another thread or vCPU makes a request (like
injecting an IRQ via kvm_make_request()), it checks the target vCPU mode
and issues an IPI via kvm_vcpu_kick() if the target is in IN_GUEST_MODE.
Because this run loop never sets vcpu->mode, kicks might be silently ignored
and the guest might not be interrupted to process requests.

>  
>  		ret = handle_exit(vcpu);
>  
>  	} while (ret > 0);

[ ... ]

> 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
> @@ -58,15 +58,18 @@ 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 WARN_ONCE here that can be triggered by userspace?
Host userspace could use KVM_SET_ONE_REG to configure an invalid vCPU state,
then invoke KVM_RUN. The hardware would immediately exit with a validity
intercept (SAE_ICPTR_VALIDITY), hitting this macro. A malicious guest or
unprivileged host userspace could trigger this to spam the host logs, or
cause a full host kernel panic if panic_on_warn is set.

>  		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);

[Severity: High]
Could this WARN_ONCE also be triggered by an unprivileged host userspace
process controlling the VM, leading to a potential kernel panic?

>  		ret = -EINVAL;
>  	}
>  	return ret;
>  }

[ ... ]

> diff --git a/arch/s390/kvm/arm64/reset.c b/arch/s390/kvm/arm64/reset.c
> --- a/arch/s390/kvm/arm64/reset.c
> +++ b/arch/s390/kvm/arm64/reset.c
> @@ -71,6 +71,6 @@ void kvm_reset_vcpu(struct kvm_vcpu *vcpu)
>  
>  int kvm_arm_vcpu_finalize(struct kvm_vcpu *vcpu, int feature)
>  {
>  	return 0;
>  }

[Severity: Medium]
Should this function validate the feature ID instead of unconditionally
returning 0? Returning success for all requests could mislead userspace into
believing unsupported features were successfully enabled, potentially causing
subtle runtime misconfigurations or migration failures.

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