Re: [PATCH v20 03/14] KVM: arm64: Manage GCS access and registers for guests

[sashiko-bot@kernel.org]

Thank you for your contribution! Sashiko AI review found 1 potential issue(s) to consider:
- [Critical] GCS register context switch logic is erroneously nested inside unrelated S1PIE and TCR2 capability checks, leading to host state leakage, asymmetrical save/restore, and guest state corruption if GCS is enabled independently.
--

commit ad11e1da6432a36cf2c3a84df41b688bfab6461b
Author: Mark Brown <broonie@kernel.org>

KVM: arm64: Manage GCS access and registers for guests

This commit introduces context switching for Guarded Control Stack (GCS)
registers and exposes them to VMMs to allow guests to use GCS features.
It also configures HCRX_EL2.GCSEn to enable GCS instruction execution in
the guest.

> diff --git a/arch/arm64/kvm/hyp/include/hyp/sysreg-sr.h b/arch/arm64/kvm/hyp/include/hyp/sysreg-sr.h
> index c382848d31947..d26802aa455e3 100644
> --- a/arch/arm64/kvm/hyp/include/hyp/sysreg-sr.h
> +++ b/arch/arm64/kvm/hyp/include/hyp/sysreg-sr.h
[ ... ]
> @@ -144,6 +158,10 @@ static inline void __sysreg_save_el1_state(struct kvm_cpu_context *ctxt)
>  		if (ctxt_has_s1pie(ctxt)) {
>  			ctxt_sys_reg(ctxt, PIR_EL1)	= read_sysreg_el1(SYS_PIR);
>  			ctxt_sys_reg(ctxt, PIRE0_EL1)	= read_sysreg_el1(SYS_PIRE0);
> +			if (ctxt_has_gcs(ctxt)) {
> +				ctxt_sys_reg(ctxt, GCSPR_EL1)	= read_sysreg_el1(SYS_GCSPR);
> +				ctxt_sys_reg(ctxt, GCSCR_EL1)	= read_sysreg_el1(SYS_GCSCR);
> +			}
>  		}

[Severity: Critical]
Does this mistakenly couple the EL1 GCS state save to the unrelated S1PIE
and TCR2 features?

Because the ctxt_has_gcs() check is nested inside ctxt_has_s1pie() (which is
itself nested under ctxt_has_tcrx() in __sysreg_save_el1_state()), if a
guest is configured with GCS enabled but S1PIE or TCR2 disabled, the
hypervisor skips saving GCSPR_EL1 and GCSCR_EL1. Since HCRX_EL2.GCSEn is
still enabled for the guest, couldn't this allow the guest to access and
potentially corrupt the host's GCS state?

[ ... ]
> @@ -243,6 +266,11 @@ static inline void __sysreg_restore_el1_state(struct kvm_cpu_context *ctxt,
>  		if (ctxt_has_s1pie(ctxt)) {
>  			write_sysreg_el1(ctxt_sys_reg(ctxt, PIR_EL1),	SYS_PIR);
>  			write_sysreg_el1(ctxt_sys_reg(ctxt, PIRE0_EL1),	SYS_PIRE0);
> +
> +			if (ctxt_has_gcs(ctxt)) {
> +				write_sysreg_el1(ctxt_sys_reg(ctxt, GCSPR_EL1),	SYS_GCSPR);
> +				write_sysreg_el1(ctxt_sys_reg(ctxt, GCSCR_EL1),	SYS_GCSCR);
> +			}
>  		}

[Severity: Critical]
Similarly for the restore path in __sysreg_restore_el1_state(), does this
miss restoring the EL1 GCS registers when S1PIE or TCR2 is disabled?

> diff --git a/arch/arm64/kvm/hyp/vhe/sysreg-sr.c b/arch/arm64/kvm/hyp/vhe/sysreg-sr.c
> index 0c4ef1ce32ae7..08b5d939a6c5a 100644
> --- a/arch/arm64/kvm/hyp/vhe/sysreg-sr.c
> +++ b/arch/arm64/kvm/hyp/vhe/sysreg-sr.c
[ ... ]
> @@ -59,10 +59,14 @@ static void __sysreg_save_vel2_state(struct kvm_vcpu *vcpu)
>  			if (ctxt_has_s1pie(&vcpu->arch.ctxt)) {
>  				__vcpu_assign_sys_reg(vcpu, PIRE0_EL2, read_sysreg_el1(SYS_PIRE0));
>  				__vcpu_assign_sys_reg(vcpu, PIR_EL2, read_sysreg_el1(SYS_PIR));
> +
> +				if (ctxt_has_gcs(&vcpu->arch.ctxt))
> +					__vcpu_assign_sys_reg(vcpu, GCSCR_EL2, read_sysreg_el1(SYS_GCSCR));
>  			}

[Severity: Critical]
Is it intentional that the GCSCR_EL2 save in __sysreg_save_vel2_state()
is nested inside the S1PIE check here?

[ ... ]
> @@ -86,6 +90,8 @@ static void __sysreg_save_vel2_state(struct kvm_vcpu *vcpu)
>  	__vcpu_assign_sys_reg(vcpu, SP_EL2,	 read_sysreg(sp_el1));
>  	__vcpu_assign_sys_reg(vcpu, ELR_EL2,	 read_sysreg_el1(SYS_ELR));
>  	__vcpu_assign_sys_reg(vcpu, SPSR_EL2,	 read_sysreg_el1(SYS_SPSR));
> +	if (ctxt_has_gcs(&vcpu->arch.ctxt))
> +		__vcpu_assign_sys_reg(vcpu, GCSPR_EL2, read_sysreg_el1(SYS_GCSPR));

[Severity: Critical]
This creates an asymmetry in __sysreg_save_vel2_state(): GCSPR_EL2 is saved
unconditionally based only on ctxt_has_gcs(), but GCSCR_EL2 is saved
conditionally based on both ctxt_has_s1pie() and ctxt_has_gcs(). Can this
cause the hypervisor to track mixed states if S1PIE is disabled?

[ ... ]
> @@ -138,6 +144,11 @@ static void __sysreg_restore_vel2_state(struct kvm_vcpu *vcpu)
>  		if (ctxt_has_s1pie(&vcpu->arch.ctxt)) {
>  			write_sysreg_el1(__vcpu_sys_reg(vcpu, PIR_EL2), SYS_PIR);
>  			write_sysreg_el1(__vcpu_sys_reg(vcpu, PIRE0_EL2), SYS_PIRE0);
> +
> +			if (ctxt_has_gcs(&vcpu->arch.ctxt)) {
> +				write_sysreg_el1(__vcpu_sys_reg(vcpu, GCSCR_EL2), SYS_GCSCR);
> +				write_sysreg_el1(__vcpu_sys_reg(vcpu, GCSPR_EL2), SYS_GCSPR);
> +			}
>  		}

[Severity: Critical]
Does this break the restore logic for GCSPR_EL2? 

In the save path above, GCSPR_EL2 is saved outside the S1PIE block, but here
in __sysreg_restore_vel2_state() it is restored inside the S1PIE block. If 
S1PIE is absent but GCS is enabled, the saved GCSPR_EL2 value is never 
restored, which could leave the hardware with an unexpected state upon exit.

-- 
Sashiko AI review · https://sashiko.dev/#/patchset/20260901-arm64-gcs-v20-0-f31750bdfadb@kernel.org?part=3