linux-kernel - Re: [PATCH v10 24/40] arm64/signal: Expose GCS state in signal frames

lists.openwall.net		lists / announce owl-users owl-dev john-users john-dev passwdqc-users yescrypt popa3d-users / oss-security kernel-hardening musl sabotage tlsify passwords / crypt-dev xvendor / Bugtraq Full-Disclosure linux-kernel linux-netdev linux-ext4 linux-hardening linux-cve-announce PHC
Open Source and information security mailing list archives

Hash Suite: Windows password security audit tool. GUI, reports in PDF.

[<prev] [next>] [<thread-prev] [thread-next>] [day] [month] [year] [list]

Message-ID: <ZsYmhHHmqSu7wWWW@arm.com>
Date: Wed, 21 Aug 2024 18:40:20 +0100
From: Catalin Marinas <catalin.marinas@....com>
To: Mark Brown <broonie@...nel.org>
Cc: Will Deacon <will@...nel.org>, Jonathan Corbet <corbet@....net>,
	Andrew Morton <akpm@...ux-foundation.org>,
	Marc Zyngier <maz@...nel.org>,
	Oliver Upton <oliver.upton@...ux.dev>,
	James Morse <james.morse@....com>,
	Suzuki K Poulose <suzuki.poulose@....com>,
	Arnd Bergmann <arnd@...db.de>, Oleg Nesterov <oleg@...hat.com>,
	Eric Biederman <ebiederm@...ssion.com>,
	Shuah Khan <shuah@...nel.org>,
	"Rick P. Edgecombe" <rick.p.edgecombe@...el.com>,
	Deepak Gupta <debug@...osinc.com>, Ard Biesheuvel <ardb@...nel.org>,
	Szabolcs Nagy <Szabolcs.Nagy@....com>, Kees Cook <kees@...nel.org>,
	"H.J. Lu" <hjl.tools@...il.com>,
	Paul Walmsley <paul.walmsley@...ive.com>,
	Palmer Dabbelt <palmer@...belt.com>,
	Albert Ou <aou@...s.berkeley.edu>,
	Florian Weimer <fweimer@...hat.com>,
	Christian Brauner <brauner@...nel.org>,
	Thiago Jung Bauermann <thiago.bauermann@...aro.org>,
	Ross Burton <ross.burton@....com>,
	linux-arm-kernel@...ts.infradead.org, linux-doc@...r.kernel.org,
	kvmarm@...ts.linux.dev, linux-fsdevel@...r.kernel.org,
	linux-arch@...r.kernel.org, linux-mm@...ck.org,
	linux-kselftest@...r.kernel.org, linux-kernel@...r.kernel.org,
	linux-riscv@...ts.infradead.org
Subject: Re: [PATCH v10 24/40] arm64/signal: Expose GCS state in signal frames

On Thu, Aug 01, 2024 at 01:06:51PM +0100, Mark Brown wrote:
> @@ -636,6 +639,81 @@ extern int restore_zt_context(struct user_ctxs *user);
>  
>  #endif /* ! CONFIG_ARM64_SME */
>  
> +#ifdef CONFIG_ARM64_GCS
> +
> +static int preserve_gcs_context(struct gcs_context __user *ctx)
> +{
> +	int err = 0;
> +	u64 gcspr;
> +
> +	/*
> +	 * We will add a cap token to the frame, include it in the
> +	 * GCSPR_EL0 we report to support stack switching via
> +	 * sigreturn.
> +	 */
> +	gcs_preserve_current_state();
> +	gcspr = current->thread.gcspr_el0 - 8;

We discussed briefly offline. Not a problem in this patch but
gcs_preserve_current_state() only saves it conditionally on GCS being
enabled for the task. However, you mentioned that the register is always
available to the user, so I'd rather change the preserving function to
save it unconditionally.

> +
> +	__put_user_error(GCS_MAGIC, &ctx->head.magic, err);
> +	__put_user_error(sizeof(*ctx), &ctx->head.size, err);
> +	__put_user_error(gcspr, &ctx->gcspr, err);
> +	__put_user_error(0, &ctx->reserved, err);
> +	__put_user_error(current->thread.gcs_el0_mode,
> +			 &ctx->features_enabled, err);
> +
> +	return err;
> +}
> +
> +static int restore_gcs_context(struct user_ctxs *user)
> +{
> +	u64 gcspr, enabled;
> +	int err = 0;
> +
> +	if (user->gcs_size != sizeof(*user->gcs))
> +		return -EINVAL;
> +
> +	__get_user_error(gcspr, &user->gcs->gcspr, err);
> +	__get_user_error(enabled, &user->gcs->features_enabled, err);
> +	if (err)
> +		return err;
> +
> +	/* Don't allow unknown modes */
> +	if (enabled & ~PR_SHADOW_STACK_SUPPORTED_STATUS_MASK)
> +		return -EINVAL;
> +
> +	err = gcs_check_locked(current, enabled);
> +	if (err != 0)
> +		return err;
> +
> +	/* Don't allow enabling */
> +	if (!task_gcs_el0_enabled(current) &&
> +	    (enabled & PR_SHADOW_STACK_ENABLE))
> +		return -EINVAL;

We don't allow enabling and that's fine but we don't restore gcspr
either with this early return.

> +
> +	/* If we are disabling disable everything */
> +	if (!(enabled & PR_SHADOW_STACK_ENABLE))
> +		enabled = 0;
> +
> +	current->thread.gcs_el0_mode = enabled;
> +
> +	/*
> +	 * We let userspace set GCSPR_EL0 to anything here, we will
> +	 * validate later in gcs_restore_signal().
> +	 */
> +	current->thread.gcspr_el0 = gcspr;
> +	write_sysreg_s(current->thread.gcspr_el0, SYS_GCSPR_EL0);

I think you should move this further up unconditionally.

-- 
Catalin