Re: [PATCH v10 23/40] arm64/signal: Set up and restore the GCS context for signal handlers

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

On Thu, Aug 01, 2024 at 01:06:50PM +0100, Mark Brown wrote:
> @@ -860,6 +892,50 @@ static int restore_sigframe(struct pt_regs *regs,
>  	return err;
>  }
>  
> +#ifdef CONFIG_ARM64_GCS
> +static int gcs_restore_signal(void)
> +{
> +	u64 gcspr_el0, cap;

Nitpick: use 'unsigned long __user *gcspr_el0' as in the
gcs_signal_entry(). It's more consistent and probably less casting.

> +	int ret;
> +
> +	if (!system_supports_gcs())
> +		return 0;
> +
> +	if (!(current->thread.gcs_el0_mode & PR_SHADOW_STACK_ENABLE))
> +		return 0;
> +
> +	gcspr_el0 = read_sysreg_s(SYS_GCSPR_EL0);
> +
> +	/*
> +	 * GCSPR_EL0 should be pointing at a capped GCS, read the cap...
> +	 */
> +	gcsb_dsync();
> +	ret = copy_from_user(&cap, (__user void*)gcspr_el0, sizeof(cap));
> +	if (ret)
> +		return -EFAULT;

Can the user change GCSPR_EL0 to a non-shadow-stack region, fake the
cap before sigreturn? copy_from_user() cannot check it's a GCS page.
Does it actually matter?

> @@ -1130,7 +1209,50 @@ static int get_sigframe(struct rt_sigframe_user_layout *user,
>  	return 0;
>  }
>  
> -static void setup_return(struct pt_regs *regs, struct k_sigaction *ka,
> +#ifdef CONFIG_ARM64_GCS
> +
> +static int gcs_signal_entry(__sigrestore_t sigtramp, struct ksignal *ksig)
> +{
> +	unsigned long __user *gcspr_el0;
> +	int ret = 0;
> +
> +	if (!system_supports_gcs())
> +		return 0;
> +
> +	if (!task_gcs_el0_enabled(current))
> +		return 0;
> +
> +	/*
> +	 * We are entering a signal handler, current register state is
> +	 * active.
> +	 */
> +	gcspr_el0 = (unsigned long __user *)read_sysreg_s(SYS_GCSPR_EL0);
> +
> +	/*
> +	 * Push a cap and the GCS entry for the trampoline onto the GCS.
> +	 */
> +	put_user_gcs((unsigned long)sigtramp, gcspr_el0 - 2, &ret);
> +	put_user_gcs(GCS_SIGNAL_CAP(gcspr_el0 - 1), gcspr_el0 - 1, &ret);
> +	if (ret != 0)
> +		return ret;

Doesn't the second put_user_gcs() override the previous ret?

> +
> +	gcsb_dsync();

Wondering if we need the barrier both for entry and restore. If the
restore happens on another CPU, we have the barriers in the context
switch code already. If it's only the kernel writing the caps with
GCSSTTR on setting up the stack and checking it on return, a single
barrier is sufficient (can be this one). If the user can write something
on the stack or maybe doing a sigreturn without fully unwinding the
stack, we may need both. Either way, it would help to add some comments
on these barriers.

-- 
Catalin