Re: [PATCH 2/2] arm64: gcs: Store GCSCRE0_EL1 values instead of userspace mode

[Catalin Marinas <catalin.marinas@arm.com>]

Hi Mark,

On Thu, Sep 17, 2026 at 11:14:17PM +0100, Mark Brown wrote:
> diff --git a/arch/arm64/include/asm/processor.h b/arch/arm64/include/asm/processor.h
> index 6dfbcacd9ba0..5b9b22ce9329 100644
> --- a/arch/arm64/include/asm/processor.h
> +++ b/arch/arm64/include/asm/processor.h
> @@ -196,7 +196,7 @@ struct thread_struct {
>  	u64			tpidr2_el0;
>  	u64			por_el0;
>  #ifdef CONFIG_ARM64_GCS
> -	unsigned int		gcs_el0_mode;
> +	unsigned int		gcscre0_el1;

u64?

> @@ -593,9 +594,7 @@ static void gcs_thread_switch(struct task_struct *next)
>  	/* GCSPR_EL0 is always readable */
>  	gcs_preserve_current_state();
>  	write_sysreg_s(next->thread.gcspr_el0, SYS_GCSPR_EL0);
> -
> -	if (current->thread.gcs_el0_mode != next->thread.gcs_el0_mode)
> -		gcs_set_el0_mode(next);
> +	write_sysreg_s(next->thread.gcscre0_el1, SYS_GCSCRE0_EL1);
>  
>  	/*
>  	 * Ensure that GCS memory effects of the 'prev' thread are

[...]

> @@ -763,7 +763,7 @@ static int restore_gcs_context(struct user_ctxs *user)
>  	if (!(enabled & PR_SHADOW_STACK_ENABLE))
>  		enabled = 0;
>  
> -	current->thread.gcs_el0_mode = enabled;
> +	gcs_set_el0_mode(current, enabled);

I think we have an existing bug here and it's more serious without this
patch. restore_gcs_context() does not update the actual register and,
with current mainline, we may even skip the hardware write in
gcs_thread_switch(). Basically one task can disable GCS for another
task. Can we get gcs_set_el0_mode() to actually update the register as
well?

On the sigreturn bug, do you recall why we thought we needed to update
the gcs_el0_mode in the signal handler (or even expose it)? Wouldn't it
be better if the signal handler did a prctl() directly if it needed to
change the mode for the current thread?

I don't think we have any precedent for allowing sigreturn to update an
EL1 sysreg. In hindsight, we shouldn't have exposed it at all but that's
ABI now. The next best thing we can do now is check the field hasn't
been modified and fault otherwise (assuming no-one does anything with
this field).

> @@ -120,29 +120,39 @@ SYSCALL_DEFINE3(map_shadow_stack, unsigned long, addr, unsigned long, size, unsi
>  	return addr;
>  }
>  
> -/*
> - * Apply the GCS mode configured for the specified task to the
> - * hardware.
> - */
> -void gcs_set_el0_mode(struct task_struct *task)
> +void gcs_set_el0_mode(struct task_struct *task, u64 flags)
>  {
> -	u64 gcscre0_el1 = GCSCRE0_EL1_nTR;
> +	task->thread.gcscre0_el1 = GCSCRE0_EL1_nTR;
>  
> -	if (task->thread.gcs_el0_mode & PR_SHADOW_STACK_ENABLE)
> -		gcscre0_el1 |= GCSCRE0_EL1_RVCHKEN | GCSCRE0_EL1_PCRSEL;
> +	if (flags & PR_SHADOW_STACK_ENABLE)
> +		task->thread.gcscre0_el1 |= GCSCRE0_EL1_RVCHKEN | GCSCRE0_EL1_PCRSEL;
>  
> -	if (task->thread.gcs_el0_mode & PR_SHADOW_STACK_WRITE)
> -		gcscre0_el1 |= GCSCRE0_EL1_STREn;
> +	if (flags & PR_SHADOW_STACK_WRITE)
> +		task->thread.gcscre0_el1 |= GCSCRE0_EL1_STREn;
>  
> -	if (task->thread.gcs_el0_mode & PR_SHADOW_STACK_PUSH)
> -		gcscre0_el1 |= GCSCRE0_EL1_PUSHMEn;
> +	if (flags & PR_SHADOW_STACK_PUSH)
> +		task->thread.gcscre0_el1 |= GCSCRE0_EL1_PUSHMEn;
> +}

Are we preemptible here? I think we are, so we should build the value
and do a WRITE_ONCE(), otherwise the switch code may get confused.

-- 
Catalin