Re: [PATCH v4 25/27] 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:
- [High] Guest vector registers are not properly reset when KVM_REQ_VCPU_RESET is processed during the KVM_RUN loop.
- [High] Userspace-reachable WARN_ONCE during validity intercept handling allows unprivileged processes to taint or panic the kernel.
- [High] Executing local_irq_enable() while in an RCU extended quiescent state (EQS) triggers lockdep warnings and system crashes.
- [High] Guest PC increment is overwritten and lost due to calling `adjust_pc` before `vcpu_put`.
- [High] Missing `vcpu_load()` and `vcpu_put()` around KVM IOCTLs modifying architectural state.
- [Medium] KVM_GET_REG_LIST ioctl returns a positive integer instead of 0 on success.
- [Medium] KVM_ARM_VCPU_FINALIZE ioctl unconditionally returns 0, failing to reject unrecognized features.
--

commit 13eb2c521ea373c254d3f7904b5979dc114e1d21
Author: Steffen Eiden <seiden@linux.ibm.com>
Subject: KVM: s390: arm64: Implement vCPU IOCTLs

Implement all vCPU IOCTLs.

> 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
[ ... ]
> @@ -375,6 +375,18 @@ 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]
Could enabling interrupts immediately after entering the guest context
trigger an RCU-idle splat?

guest_enter_irqoff() places the CPU into an RCU extended quiescent state
(EQS). Calling local_irq_enable() inside this state violates the API
contract since it makes the kernel preemptible while RCU is not watching,
which can lead to lockdep warnings and system stalls.

[ ... ]
> @@ -396,6 +408,61 @@ 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;
[ ... ]
>  	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 (ret > 0)
>  			ret = check_vcpu_requests(vcpu);

[Severity: High]
If a KVM_REQ_VCPU_RESET is pending and processed by check_vcpu_requests(),
the memory copy of the core registers is zeroed out by kvm_reset_vcpu().
However, the hardware CPU vector registers were already loaded before the
loop via load_vx_regs().

When the loop exits, won't save_vx_regs() copy the stale hardware state back
into memory, completely overriding the reset we just performed?

[ ... ]
>  		arm_vcpu_run(vcpu);
>  
>  		ret = handle_exit(vcpu);
>  
>  	} while (ret > 0);
>  
>  	kvm_sigset_deactivate(vcpu);
>  out:
>  	if (unlikely(vcpu_get_flag(vcpu, INCREMENT_PC)))
>  		adjust_pc(vcpu);
>  
>  	save_vx_regs(vcpu->arch.ctxt.vregs);
>  	kernel_fpu_end(&fpu_save, KERNEL_FPC | KERNEL_VXR);
>  	vcpu_put(vcpu);

[Severity: High]
Does this sequence overwrite the PC increment?

adjust_pc() modifies the PC in the vCPU's memory state, but the immediate
subsequent call to vcpu_put() executes the stiasrm hardware instruction.
This dumps the current internal hardware register state (with the
un-incremented PC) into the exact same memory save area.

Could this permanently overwrite the software increment and cause infinite
guest execution loops?

[ ... ]
> @@ -463,6 +530,95 @@ long kvm_arch_vcpu_ioctl(struct file *filp, unsigned int ioctl, unsigned long ar
>  {
>  	struct kvm_vcpu *vcpu = filp->private_data;
>  	void __user *argp = (void __user *)arg;
>  	struct kvm_device_attr attr;
>  	int ret;
>  
>  	switch (ioctl) {

[Severity: High]
Are we missing calls to vcpu_load() and vcpu_put() around these IOCTLs?

Operations like KVM_SET_ONE_REG and KVM_ARM_VCPU_INIT directly modify or
read the vCPU architectural state. The KVM subsystem mandates that IOCTLs
modifying architectural state must use these wrappers to manage vCPU
attachment.

Without them, could this lead to hardware state synchronization issues or
leaked preempt notifiers?

[ ... ]
>  	case KVM_GET_REG_LIST: {
>  		struct kvm_reg_list __user *user_list = argp;
>  		struct kvm_reg_list reg_list;
>  		unsigned int n;
[ ... ]
>  		n = reg_list.n;
>  		reg_list.n = kvm_arm_num_regs(vcpu);
>  		if (copy_to_user(user_list, &reg_list, sizeof(reg_list)))
>  			break;
>  		ret = -E2BIG;
>  		if (n < reg_list.n)
>  			break;
>  		ret = kvm_arm_copy_reg_indices(vcpu, user_list->reg);
>  		break;
>  	}

[Severity: Medium]
Should this explicitly return 0 on success?

kvm_arm_copy_reg_indices() returns the positive number of registers copied.
Passing this positive count directly back to userspace goes against the KVM
ABI requirement that this ioctl returns 0 on success.

Could this cause userspace programs like QEMU to misinterpret a successful
call as an error?

[ ... ]
> 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
[ ... ]
> @@ -55,6 +57,35 @@ exit_handle_fn arm_exit_handlers[] = {
>  	[ESR_ELx_EC_HVC64]	= handle_hvc,
>  };
>  
> +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;

[Severity: High]
Is it safe to have a WARN_ONCE here?

Unprivileged host userspace can set arbitrary guest register states via
KVM_SET_ONE_REG prior to calling KVM_RUN. This allows a malicious userspace
process to intentionally construct an invalid CPU state that hardware
rejects.

Would this allow an unprivileged process to drive this warning condition and
create a host-side denial of service vector if panic_on_warn is enabled?

[ ... ]
> diff --git a/arch/s390/kvm/arm64/reset.c b/arch/s390/kvm/arm64/reset.c
> --- /dev/null
> +++ b/arch/s390/kvm/arm64/reset.c
[ ... ]
> +int kvm_arm_vcpu_finalize(struct kvm_vcpu *vcpu, int feature)
> +{
> +	return 0;
> +}

[Severity: Medium]
Should this validate the feature parameter instead of unconditionally
returning 0?

The KVM API specifies that finalizing an unsupported or invalid feature must
fail with -EINVAL or -EPERM. Returning 0 silently acknowledges invalid
feature finalizations.

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