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

[sashiko-bot@kernel.org]

Thank you for your contribution! Sashiko AI review found 3 potential issue(s) to consider:
- [High] EL1 Guarded Control Stack (GCS) registers are incorrectly nested inside S1PIE and TCRX capability checks.
- [High] EL2 Guarded Control Stack (GCS) registers are incorrectly nested inside TCRX capability checks, with asymmetric save/restore logic for GCSPR_EL2.
- [Medium] GCSPR_EL0 and GCSCRE0_EL1 are incorrectly classified as SR_LOC_MEMORY when in a nested hypervisor context.
--

--- Patch [3]: [PATCH v18 03/14] KVM: arm64: Manage GCS access and registers for guests ---
commit 408aec21b1f27709492e1981fcc42dc3f0f70337
Author: Mark Brown <broonie@kernel.org>
Subject: KVM: arm64: Manage GCS access and registers for guests

This patch adds support for exposing and context switching Guarded Control
Stack (GCS) registers for guests. It also configures HCRX_EL2.GCSEn to
enable GCS instructions.

> diff --git a/arch/arm64/kvm/hyp/include/hyp/sysreg-sr.h b/arch/arm64/kvm/hyp/include/hyp/sysreg-sr.h
> index a17cbe7582de9..053d7b3c5104d 100644
> --- a/arch/arm64/kvm/hyp/include/hyp/sysreg-sr.h
> +++ b/arch/arm64/kvm/hyp/include/hyp/sysreg-sr.h
> @@ -144,6 +161,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: High]
Are the EL1 Guarded Control Stack (GCS) registers incorrectly nested inside
the S1PIE and TCRX capability checks?

FEAT_GCS, FEAT_S1PIE, and FEAT_TCR2 (TCRX) are architecturally independent CPU
features. If a guest is configured with GCS enabled but S1PIE or TCRX
disabled, the host will entirely skip saving the guest's GCSPR_EL1 and
GCSCR_EL1 registers during context switches.

Does this leave stale host or prior-guest state in these hardware registers,
leading to guest state corruption or cross-guest information leaks?

> @@ -239,6 +265,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: High]
Similarly, is the restoration of GCSPR_EL1 and GCSCR_EL1 incorrectly nested
inside the S1PIE and TCRX checks, causing restoration to be skipped when
these independent features are disabled?

> diff --git a/arch/arm64/kvm/hyp/vhe/sysreg-sr.c b/arch/arm64/kvm/hyp/vhe/sysreg-sr.c
> index be685b63e8cf2..2431c18f57928 100644
> --- a/arch/arm64/kvm/hyp/vhe/sysreg-sr.c
> +++ b/arch/arm64/kvm/hyp/vhe/sysreg-sr.c
> @@ -61,6 +61,9 @@ static void __sysreg_save_vel2_state(struct kvm_vcpu *vcpu)
>  
>  			if (ctxt_has_s1poe(&vcpu->arch.ctxt))
>  				__vcpu_assign_sys_reg(vcpu, POR_EL2, read_sysreg_el1(SYS_POR));
> +
> +			if (ctxt_has_gcs(&vcpu->arch.ctxt))
> +				__vcpu_assign_sys_reg(vcpu, GCSCR_EL2, read_sysreg_el1(SYS_GCSCR));
>  		}
>  
>  		/*
> @@ -77,6 +80,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));
>  
>  	if (ctxt_has_sctlr2(&vcpu->arch.ctxt))
>  		__vcpu_assign_sys_reg(vcpu, SCTLR2_EL2, read_sysreg_el1(SYS_SCTLR2));

[Severity: High]
Is the GCSCR_EL2 save erroneously nested inside the ctxt_has_tcrx() block,
while GCSPR_EL2 is saved asymmetrically outside of it?

Since GCS does not depend on TCR2 (TCRX), a nested hypervisor running with GCS
but without TCR2 will not have GCSCR_EL2 saved correctly. Furthermore,
the asymmetric save logic between the two registers creates an unbalanced
cycle that can lead to corrupted nested guest states.

> @@ -133,6 +138,11 @@ static void __sysreg_restore_vel2_state(struct kvm_vcpu *vcpu)
>  
>  		if (ctxt_has_s1poe(&vcpu->arch.ctxt))
>  			write_sysreg_el1(__vcpu_sys_reg(vcpu, POR_EL2), SYS_POR);
> +
> +		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);
> +		}
>  	}
>  
>  	write_sysreg_el1(__vcpu_sys_reg(vcpu, ESR_EL2),		SYS_ESR);

[Severity: High]
Are both EL2 GCS registers erroneously restored inside the ctxt_has_tcrx()
block? GCSPR_EL2 was saved unconditionally outside this block in
__sysreg_save_vel2_state(), meaning it might be saved but never restored if
TCRX is absent.

> diff --git a/arch/arm64/kvm/sys_regs.c b/arch/arm64/kvm/sys_regs.c
> index 5d5c579d45790..d18ada283809e 100644
> --- a/arch/arm64/kvm/sys_regs.c
> +++ b/arch/arm64/kvm/sys_regs.c
> @@ -109,6 +109,10 @@ static enum sr_loc_attr locate_direct_register(const struct kvm_vcpu *vcpu,
>  	case PIR_EL1:
>  	case PIRE0_EL1:
>  	case POR_EL1:
> +	case GCSCRE0_EL1:
> +	case GCSPR_EL0:
> +	case GCSCR_EL1:
> +	case GCSPR_EL1:
>  	case ESR_EL1:
>  	case AFSR0_EL1:
>  	case AFSR1_EL1:

[Severity: Medium]
Are GCSPR_EL0 and GCSCRE0_EL1 incorrectly classified as SR_LOC_MEMORY when in
a nested hypervisor context?

These registers are eagerly loaded onto the physical hardware via
__sysreg_save_user_state() and __sysreg_restore_user_state(), and they have no
EL2 aliases. They are always resident on the physical CPU when the vCPU is
running, regardless of whether the guest is in vEL1 or vEL2 context.

Classifying them as SR_LOC_MEMORY when is_hyp_ctxt(vcpu) is true means any
host-side access will read the stale vcpu->arch.ctxt.sys_regs array instead of
the active hardware register. Should they unconditionally return SR_LOC_LOADED
like TPIDR_EL0?

-- 
Sashiko AI review · https://sashiko.dev/#/patchset/20260803-arm64-gcs-v18-0-fd8c40f5ad88@kernel.org?part=3