Re: [PATCH v3 19/23] KVM: s390: arm64: Implement exception injection

[sashiko-bot@kernel.org]

Thank you for your contribution! Sashiko AI review found 2 potential
issues to consider:
- [High] KVM: s390: arm64: preemptible smp_processor_id() and migration
  race in sysreg access
- [Medium] KVM: s390: arm64: unsigned gltl overflow in inject_abt64()
--

commit 9004b39a5f6b970883241ce75cb94bfa9bdd2fe1
Author: Steffen Eiden <seiden@linux.ibm.com>

KVM: s390: arm64: Implement exception injection

This commit adds guest exception entry and fault injection support for
arm64 guests running under KVM on s390.

> diff --git a/arch/s390/kvm/arm64/arm.c b/arch/s390/kvm/arm64/arm.c
> index 4de0f17052cb8..91e9f4a75ce73 100644
> --- a/arch/s390/kvm/arm64/arm.c
> +++ b/arch/s390/kvm/arm64/arm.c
[ ... ]
> @@ -511,8 +503,9 @@ int kvm_arch_vcpu_ioctl_run(struct kvm_vcpu *vcpu)
>  
>  	kvm_sigset_deactivate(vcpu);
>  out:
> -	if (unlikely(vcpu_get_flag(vcpu, INCREMENT_PC)))
> -		adjust_pc(vcpu);
> +	if (unlikely(vcpu_get_flag(vcpu, PENDING_EXCEPTION) ||
> +		     vcpu_get_flag(vcpu, INCREMENT_PC)))
> +		kvm_adjust_pc(vcpu);

[Severity: High]
Does accessing save-area registers with preemption enabled here and during
handle_exit() trigger a debug_smp_processor_id() splat and race with host
CPU migration?

At the out label (before vcpu_put()) and inside handle_exit(), the vCPU is
loaded (vcpu_is_loaded(vcpu) is true) while IRQs and preemption are
enabled.  When an exception is injected (for example via
kvm_inject_undefined() -> kvm_inject_sync() or inject_abt64()) or
delivered here via kvm_adjust_pc() -> kvm_inject_exception() ->
enter_exception64(), vcpu_read_sys_reg() and vcpu_write_sys_reg() access
VBAR_EL1, SCTLR_EL1, FAR_EL1, and ESR_EL1 through _vcpu_read_sys_reg() and
_vcpu_write_sys_reg():

arch/s390/include/asm/kvm_host_arm64.h:_vcpu_read_sys_reg()
	KVM_BUG_ON(vcpu_is_loaded(C) && smp_processor_id() != (C)->cpu, (C)->kvm);

Calling smp_processor_id() in preemptible context triggers a BUG splat
when CONFIG_DEBUG_PREEMPT is enabled, and if the vCPU task is preempted
and migrated between reading smp_processor_id() and reading vcpu->cpu,
KVM_BUG_ON() fires and terminates the VM:

CPU0                              CPU1
handle_exit() / kvm_adjust_pc()
  vcpu_read_sys_reg(vcpu, VBAR_EL1)
    smp_processor_id() -> 0
  <preempted and migrated>
                                  kvm_sched_in()
                                    kvm_arch_vcpu_load(vcpu, 1)
                                      vcpu->cpu = 1
                                  vcpu->cpu -> 1 != 0
                                  KVM_BUG_ON()  <-- kills VM

> diff --git a/arch/s390/kvm/arm64/inject_fault.c b/arch/s390/kvm/arm64/inject_fault.c
> index 425dbeaa421c6..33f40c5188c98 100644
> --- a/arch/s390/kvm/arm64/inject_fault.c
> +++ b/arch/s390/kvm/arm64/inject_fault.c
> @@ -2,6 +2,38 @@
>  
>  #include <arm64/kvm_emulate.h>
>  
> +static void pend_sync_exception(struct kvm_vcpu *vcpu)
> +{
> +	kvm_pend_exception(vcpu, EXCEPT_AA64_EL1_SYNC);
> +}
> +
> +static void inject_abt64(struct kvm_vcpu *vcpu, bool is_iabt, unsigned long addr)
> +{
> +	unsigned long cpsr = *vcpu_cpsr(vcpu);
> +	bool is_aarch32 = vcpu_mode_is_32bit(vcpu);
> +	u64 esr = 0;
> +
> +	if (kvm_vcpu_abt_iss1tw(vcpu))
> +		esr |= ESR_ELx_FSC_SEA_TTW(kvm_vcpu_abt_gltl(vcpu));
> +	else
> +		esr |= ESR_ELx_FSC_EXTABT;

[Severity: Medium]
Can ESR_ELx_FSC_SEA_TTW(kvm_vcpu_abt_gltl(vcpu)) overflow the 6-bit FSC
field into bit 8 of ESR_EL1 on a level -1 translation table walk?

vcpu->arch.sae_block.hai.gltl is declared as u8, so kvm_vcpu_abt_gltl()
zero-extends a two's-complement -1 (0xff) to 255 instead of -1.
ESR_ELx_FSC_SEA_TTW(255) then evaluates to 0x113 (0x14 + 255), which sets
bit 8 (ESR_ELx_CM for data aborts or a RES0 bit for instruction aborts) in
esr, and kvm_inject_size_fault() only masks off GENMASK_ULL(5, 0)
afterwards.

-- 
Sashiko AI review · https://sashiko.dev/#/patchset/20261106-sysreg-v3-0-75e95980fb10@linux.ibm.com?part=19