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

Thank you for your contribution! Sashiko AI review found 3 potential issue(s) to consider:
- [High] GCS register context switch routines are incorrectly nested inside independent CPU feature checks (S1PIE and TCR2/TCRX), causing state corruption.
- [High] User-state GCS registers (`GCSPR_EL0` and `GCSCRE0_EL1`) are omitted from `locate_direct_register()` and live CPU accessors, breaking `KVM_SET_ONE_REG`.
- [High] GCS is explicitly masked out of the ID_AA64PFR1_EL1 register, making it impossible to expose to guests.
--

--- Patch [3]: [PATCH v17 03/14] KVM: arm64: Manage GCS access and registers for guests ---
commit 886331199ccf7c07df03690da6445af3ec9f7d3d
Author: Mark Brown <[email protected]>
Subject: KVM: arm64: Manage GCS access and registers for guests

This patch introduces KVM support for the Guarded Control Stack (GCS) extension.
It handles context switching of GCS registers for guests and configures
HCRX_EL2.GCSEn to enable GCS instruction execution.

> 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]
Does this code inadvertently bypass GCS saves when S1PIE is disabled?

Because the ctxt_has_gcs() block is nested inside if (ctxt_has_s1pie()), 
a VM with GCS enabled but S1PIE disabled will fail to save these registers, 
leading to state corruption across context switches.

[ ... ]
> @@ -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, does this skip restoring GCS registers if S1PIE is 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));
>  		}
>  

[Severity: High]
Is GCSCR_EL2 supposed to be saved only when TCRX is present? 

Here, the GCSCR_EL2 save logic is nested inside the if (ctxt_has_tcrx())
block, but below, GCSPR_EL2 is saved unconditionally outside of it.

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

[ ... ]
> @@ -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);
> +		}
>  	}
>  

[Severity: High]
Does this code inadvertently skip restoring both GCSPR_EL2 and GCSCR_EL2 
when TCRX is disabled?

Both are improperly nested inside the ctxt_has_tcrx() block here, meaning
they won't be restored on context switch for VMs without TCRX.

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

[Severity: High]
Should the user-state GCS registers (GCSPR_EL0 and GCSCRE0_EL1) be explicitly
added here to return SR_LOC_LOADED?

Because they are omitted from locate_direct_register(), they fall back to
SR_LOC_MEMORY. When userspace uses KVM_SET_ONE_REG, vcpu_put() will overwrite
the updated memory array with the unmodified hardware state, breaking state
restoration.

Likewise, it appears they are also missing from the live CPU accessors:
read_sr_from_cpu() and write_sr_to_cpu().

[Severity: High]
Is the GCS feature bit still explicitly masked out of the ID_AA64PFR1_EL1
register limit?

Looking at sanitise_id_aa64pfr1_el1() in arch/arm64/kvm/sys_regs.c, it appears
the GCS bit is unconditionally cleared. If so, when the VMM tries to enable
GCS via KVM_SET_ONE_REG, arm64_check_features() will validate against a limit
of 0 and reject it with -E2BIG, making it impossible to expose GCS to guests.

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