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

Thank you for your contribution! Sashiko AI review found 2 potential issue(s) to consider:
- [Critical] GCS system register save/restore logic is incorrectly nested inside unrelated S1PIE and TCRX feature checks, causing guest state corruption and potential host control-flow hijack.
- [Critical] Calling ctxt_has_gcs() on the host context calculates a bogus vCPU pointer and dereferences arbitrary memory, crashing the host kernel on VCPU load/put.
--

--- Patch [3]: [PATCH v19 03/14] KVM: arm64: Manage GCS access and registers for guests ---
commit 7b54a8ce806322bdd9585098668578c6af7e75b8
Author: Mark Brown <[email protected]>
Date:   Wed Aug 12 19:12:02 2026 +0000

    KVM: arm64: Manage GCS access and registers for guests
    
    GCS introduces a number of system registers, on systems with GCS we need
    to context switch them and expose them to VMMs to allow guests to use
    GCS.
    
    In order to allow guests to use GCS we also need to configure
    HCRX_EL2.GCSEn, if this is not set GCS instructions will be noops and
    CHKFEAT will report GCS as disabled.

> 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
> @@ -67,6 +68,11 @@ static inline void __sysreg_save_user_state(struct kvm_cpu_context *ctxt)
>  {
>  	ctxt_sys_reg(ctxt, TPIDR_EL0)	= read_sysreg(tpidr_el0);
>  	ctxt_sys_reg(ctxt, TPIDRRO_EL0)	= read_sysreg(tpidrro_el0);
> +
> +	if (ctxt_has_gcs(ctxt)) {
> +		ctxt_sys_reg(ctxt, GCSPR_EL0) = read_sysreg_s(SYS_GCSPR_EL0);
> +		ctxt_sys_reg(ctxt, GCSCRE0_EL1)	= read_sysreg_s(SYS_GCSCRE0_EL1);
> +	}
>  }

[Severity: Critical]
Does this cause a host kernel panic when called on the host context?

__sysreg_save_user_state() is called on the host context during vcpu_load()
and vcpu_put(). The host_ctxt is embedded inside the per-CPU struct
kvm_host_data, not a kvm_vcpu.

When ctxt_has_gcs(ctxt) evaluates ctxt_to_vcpu(ctxt), the container_of() math
will yield a bogus kvm_vcpu pointer, leading to an out-of-bounds memory read
when it dereferences vcpu->kvm to check the features.

[ ... ]
> @@ -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: Critical]
Should the GCS register saving be conditionally nested inside the S1PIE
feature check?

If a KVM guest is configured with GCS enabled but without S1PIE, it appears
the GCS registers will not be saved or restored. This could leave stale GCS
register values in hardware across context switches, potentially allowing
guest-to-host privilege escalation if the host resumes with a guest-controlled
GCSPR_EL1.

[ ... ]
> @@ -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: Critical]
Similarly, the GCS register restoration appears to be incorrectly tied to the
S1PIE feature check here.

> diff --git a/arch/arm64/kvm/hyp/vhe/sysreg-sr.c b/arch/arm64/kvm/hyp/vhe/sysreg-sr.c
> index be685b63e8cf2..4f775545c71ba 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: Critical]
Is there a structural asymmetry being introduced here? 

GCSCR_EL2 save is nested inside the TCRX feature check, while GCSPR_EL2 is
saved entirely independently later in this function. This ties GCSCR_EL2
saving to an unrelated feature.

[ ... ]
> @@ -129,6 +134,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 also tie the GCS restoration to the unrelated S1PIE and TCRX
features?

Both GCSCR_EL2 and GCSPR_EL2 restorations are deeply nested inside the S1PIE
feature check here, breaking symmetry with the save path above and requiring
S1PIE to be enabled for GCS registers to be restored.

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