[PATCH v2 0/4] arm64/signal: Support TPIDR2

[PATCH v2 0/4] arm64/signal: Support TPIDR2

[Mark Brown]

When SME support was merged support for TPIDR2 in signal frames was
omitted, meaning that it was not possible for signal handers to inspect
or modify it. This will present an issue for programs using signals to
implement lightweight threads so let's provide access to TPIDR2 in
signal handlers.

Implement a new record type for TPIDR2 using the same format as we use
for ESR and add coverage to make sure that this appears in the signal
context as expected.  Due to TPIDR2 being reserved for libc we only
validate that the value is unchanged, meaning we're likely to just be
validating the default value of 0 on current systems. I have tested with
a modified version that sets an explicit value.

v2:
 - Rebase onto v6.1-rc3.
 - Change the signal frame magic to 0x54504902 (TPI\x02).

Mark Brown (4):
  arm64/sme: Document ABI for TPIDR2 signal information
  arm64/signal: Include TPIDR2 in the signal context
  kselftest/arm64: Add TPIDR2 to the set of known signal context records
  kselftest/arm64: Add test case for TPIDR2 signal frame records

 Documentation/arm64/sme.rst                   |  3 +
 arch/arm64/include/uapi/asm/sigcontext.h      |  8 ++
 arch/arm64/kernel/signal.c                    | 59 ++++++++++++
 .../testing/selftests/arm64/signal/.gitignore |  1 +
 .../arm64/signal/testcases/testcases.c        |  4 +
 .../arm64/signal/testcases/tpidr2_siginfo.c   | 90 +++++++++++++++++++
 6 files changed, 165 insertions(+)
 create mode 100644 tools/testing/selftests/arm64/signal/testcases/tpidr2_siginfo.c


base-commit: 30a0b95b1335e12efef89dd78518ed3e4a71a763
-- 
2.30.2


_______________________________________________
linux-arm-kernel mailing list
linux-arm-kernel@lists.infradead.org
http://lists.infradead.org/mailman/listinfo/linux-arm-kernel

[PATCH v2 1/4] arm64/sme: Document ABI for TPIDR2 signal information

[Mark Brown]

In order to allow access to TPIDR2 from signal handlers we need to add it
to the signal context, document that we are doing so.

Signed-off-by: Mark Brown <broonie@kernel.org>
---
 Documentation/arm64/sme.rst | 3 +++
 1 file changed, 3 insertions(+)

diff --git a/Documentation/arm64/sme.rst b/Documentation/arm64/sme.rst
index 16d2db4c2e2e..36b08a105dd1 100644
--- a/Documentation/arm64/sme.rst
+++ b/Documentation/arm64/sme.rst
@@ -111,6 +111,9 @@ be zeroed.
 
 * Signal handlers are invoked with streaming mode and ZA disabled.
 
+* A new signal frame record TPIDR2_MAGIC is added formatted as a struct
+  tpidr2_context to allow access to TPIDR2_EL0 from signal handlers.
+
 * A new signal frame record za_context encodes the ZA register contents on
   signal delivery. [1]
 
-- 
2.30.2


_______________________________________________
linux-arm-kernel mailing list
linux-arm-kernel@lists.infradead.org
http://lists.infradead.org/mailman/listinfo/linux-arm-kernel

[PATCH v2 2/4] arm64/signal: Include TPIDR2 in the signal context

[Mark Brown]

Add a new signal frame record for TPIDR2 using the same format as we
already use for ESR with different magic, a header with the value from the
register appended as the only data. If SME is supported then this record is
always included.

Signed-off-by: Mark Brown <broonie@kernel.org>
Reviewed-by: Szabolcs Nagy <szabolcs.nagy@arm.com>
---
 arch/arm64/include/uapi/asm/sigcontext.h |  8 ++++
 arch/arm64/kernel/signal.c               | 59 ++++++++++++++++++++++++
 2 files changed, 67 insertions(+)

diff --git a/arch/arm64/include/uapi/asm/sigcontext.h b/arch/arm64/include/uapi/asm/sigcontext.h
index 4aaf31e3bf16..46ce097ca8c0 100644
--- a/arch/arm64/include/uapi/asm/sigcontext.h
+++ b/arch/arm64/include/uapi/asm/sigcontext.h
@@ -140,6 +140,14 @@ struct sve_context {
 
 #define SVE_SIG_FLAG_SM	0x1	/* Context describes streaming mode */
 
+/* TPIDR2_EL0 context */
+#define TPIDR2_MAGIC	0x54504902
+
+struct tpidr2_context {
+	struct _aarch64_ctx head;
+	__u64 tpidr2;
+};
+
 #define ZA_MAGIC	0x54366345
 
 struct za_context {
diff --git a/arch/arm64/kernel/signal.c b/arch/arm64/kernel/signal.c
index 9ad911f1647c..ac4fb42a9613 100644
--- a/arch/arm64/kernel/signal.c
+++ b/arch/arm64/kernel/signal.c
@@ -56,6 +56,7 @@ struct rt_sigframe_user_layout {
 	unsigned long fpsimd_offset;
 	unsigned long esr_offset;
 	unsigned long sve_offset;
+	unsigned long tpidr2_offset;
 	unsigned long za_offset;
 	unsigned long extra_offset;
 	unsigned long end_offset;
@@ -219,6 +220,7 @@ static int restore_fpsimd_context(struct fpsimd_context __user *ctx)
 struct user_ctxs {
 	struct fpsimd_context __user *fpsimd;
 	struct sve_context __user *sve;
+	struct tpidr2_context __user *tpidr2;
 	struct za_context __user *za;
 };
 
@@ -358,6 +360,32 @@ extern int preserve_sve_context(void __user *ctx);
 
 #ifdef CONFIG_ARM64_SME
 
+static int preserve_tpidr2_context(struct tpidr2_context __user *ctx)
+{
+	int err = 0;
+
+	current->thread.tpidr2_el0 = read_sysreg_s(SYS_TPIDR2_EL0);
+
+	__put_user_error(TPIDR2_MAGIC, &ctx->head.magic, err);
+	__put_user_error(sizeof(*ctx), &ctx->head.size, err);
+	__put_user_error(current->thread.tpidr2_el0, &ctx->tpidr2, err);
+
+	return err;
+}
+
+static int restore_tpidr2_context(struct user_ctxs *user)
+{
+	u64 tpidr2_el0;
+	int err = 0;
+
+	/* Magic and size were validated deciding to call this function */
+	__get_user_error(tpidr2_el0, &user->tpidr2->tpidr2, err);
+	if (!err)
+		current->thread.tpidr2_el0 = tpidr2_el0;
+
+	return err;
+}
+
 static int preserve_za_context(struct za_context __user *ctx)
 {
 	int err = 0;
@@ -447,6 +475,8 @@ static int restore_za_context(struct user_ctxs *user)
 #else /* ! CONFIG_ARM64_SME */
 
 /* Turn any non-optimised out attempts to use these into a link error: */
+extern int preserve_tpidr2_context(void __user *ctx);
+extern int restore_tpidr2_context(struct user_ctxs *user);
 extern int preserve_za_context(void __user *ctx);
 extern int restore_za_context(struct user_ctxs *user);
 
@@ -465,6 +495,7 @@ static int parse_user_sigframe(struct user_ctxs *user,
 
 	user->fpsimd = NULL;
 	user->sve = NULL;
+	user->tpidr2 = NULL;
 	user->za = NULL;
 
 	if (!IS_ALIGNED((unsigned long)base, 16))
@@ -531,6 +562,19 @@ static int parse_user_sigframe(struct user_ctxs *user,
 			user->sve = (struct sve_context __user *)head;
 			break;
 
+		case TPIDR2_MAGIC:
+			if (!system_supports_sme())
+				goto invalid;
+
+			if (user->tpidr2)
+				goto invalid;
+
+			if (size != sizeof(*user->tpidr2))
+				goto invalid;
+
+			user->tpidr2 = (struct tpidr2_context __user *)head;
+			break;
+
 		case ZA_MAGIC:
 			if (!system_supports_sme())
 				goto invalid;
@@ -663,6 +707,9 @@ static int restore_sigframe(struct pt_regs *regs,
 			err = restore_fpsimd_context(user.fpsimd);
 	}
 
+	if (err == 0 && system_supports_sme() && user.tpidr2)
+		err = restore_tpidr2_context(&user);
+
 	if (err == 0 && system_supports_sme() && user.za)
 		err = restore_za_context(&user);
 
@@ -757,6 +804,11 @@ static int setup_sigframe_layout(struct rt_sigframe_user_layout *user,
 		else
 			vl = task_get_sme_vl(current);
 
+		err = sigframe_alloc(user, &user->tpidr2_offset,
+				     sizeof(struct tpidr2_context));
+		if (err)
+			return err;
+
 		if (thread_za_enabled(&current->thread))
 			vq = sve_vq_from_vl(vl);
 
@@ -814,6 +866,13 @@ static int setup_sigframe(struct rt_sigframe_user_layout *user,
 		err |= preserve_sve_context(sve_ctx);
 	}
 
+	/* TPIDR2 if supported */
+	if (system_supports_sme() && err == 0) {
+		struct tpidr2_context __user *tpidr2_ctx =
+			apply_user_offset(user, user->tpidr2_offset);
+		err |= preserve_tpidr2_context(tpidr2_ctx);
+	}
+
 	/* ZA state if present */
 	if (system_supports_sme() && err == 0 && user->za_offset) {
 		struct za_context __user *za_ctx =
-- 
2.30.2


_______________________________________________
linux-arm-kernel mailing list
linux-arm-kernel@lists.infradead.org
http://lists.infradead.org/mailman/listinfo/linux-arm-kernel

[PATCH v2 3/4] kselftest/arm64: Add TPIDR2 to the set of known signal context records

[Mark Brown]

When validating the set of signal context records check that any TPIDR2
record has the correct size, also suppressing warnings due to seeing an
unknown record type.

Signed-off-by: Mark Brown <broonie@kernel.org>
---
 tools/testing/selftests/arm64/signal/testcases/testcases.c | 4 ++++
 1 file changed, 4 insertions(+)

diff --git a/tools/testing/selftests/arm64/signal/testcases/testcases.c b/tools/testing/selftests/arm64/signal/testcases/testcases.c
index e1c625b20ac4..f98cc081aa95 100644
--- a/tools/testing/selftests/arm64/signal/testcases/testcases.c
+++ b/tools/testing/selftests/arm64/signal/testcases/testcases.c
@@ -158,6 +158,10 @@ bool validate_reserved(ucontext_t *uc, size_t resv_sz, char **err)
 			if (head->size != sizeof(struct esr_context))
 				*err = "Bad size for esr_context";
 			break;
+		case TPIDR2_MAGIC:
+			if (head->size != sizeof(struct tpidr2_context))
+				*err = "Bad size for tpidr2_context";
+			break;
 		case SVE_MAGIC:
 			if (flags & SVE_CTX)
 				*err = "Multiple SVE_MAGIC";
-- 
2.30.2


_______________________________________________
linux-arm-kernel mailing list
linux-arm-kernel@lists.infradead.org
http://lists.infradead.org/mailman/listinfo/linux-arm-kernel

[PATCH v2 4/4] kselftest/arm64: Add test case for TPIDR2 signal frame records

[Mark Brown]

Ensure that we get signal context for TPIDR2 if and only if SME is present
on the system. Since TPIDR2 is owned by libc we merely validate that the
value is whatever it was set to, this isn't ideal since it's likely to
just be the default of 0 with current systems but it avoids future false
positives.

Signed-off-by: Mark Brown <broonie@kernel.org>
---
 .../testing/selftests/arm64/signal/.gitignore |  1 +
 .../arm64/signal/testcases/tpidr2_siginfo.c   | 90 +++++++++++++++++++
 2 files changed, 91 insertions(+)
 create mode 100644 tools/testing/selftests/arm64/signal/testcases/tpidr2_siginfo.c

diff --git a/tools/testing/selftests/arm64/signal/.gitignore b/tools/testing/selftests/arm64/signal/.gitignore
index e8d2b57f73ec..e1b6c4d961b5 100644
--- a/tools/testing/selftests/arm64/signal/.gitignore
+++ b/tools/testing/selftests/arm64/signal/.gitignore
@@ -4,5 +4,6 @@ fake_sigreturn_*
 sme_*
 ssve_*
 sve_*
+tpidr2_siginfo
 za_*
 !*.[ch]
diff --git a/tools/testing/selftests/arm64/signal/testcases/tpidr2_siginfo.c b/tools/testing/selftests/arm64/signal/testcases/tpidr2_siginfo.c
new file mode 100644
index 000000000000..6a2c82bf7ead
--- /dev/null
+++ b/tools/testing/selftests/arm64/signal/testcases/tpidr2_siginfo.c
@@ -0,0 +1,90 @@
+// SPDX-License-Identifier: GPL-2.0
+/*
+ * Copyright (C) 2022 ARM Limited
+ *
+ * Verify that the TPIDR2 register context in signal frames is set up as
+ * expected.
+ */
+
+#include <signal.h>
+#include <ucontext.h>
+#include <sys/auxv.h>
+#include <sys/prctl.h>
+#include <unistd.h>
+#include <asm/sigcontext.h>
+
+#include "test_signals_utils.h"
+#include "testcases.h"
+
+static union {
+	ucontext_t uc;
+	char buf[1024 * 128];
+} context;
+
+#define SYS_TPIDR2 "S3_3_C13_C0_5"
+
+static uint64_t get_tpidr2(void)
+{
+	uint64_t val;
+
+	asm volatile (
+		"mrs	%0, " SYS_TPIDR2 "\n"
+		: "=r"(val)
+		:
+		: "cc");
+
+	return val;
+}
+
+int tpidr2_present(struct tdescr *td, siginfo_t *si, ucontext_t *uc)
+{
+	struct _aarch64_ctx *head = GET_BUF_RESV_HEAD(context);
+	struct tpidr2_context *tpidr2_ctx;
+	size_t offset;
+	bool in_sigframe;
+	bool have_sme;
+	__u64 orig_tpidr2;
+
+	have_sme = getauxval(AT_HWCAP2) & HWCAP2_SME;
+	if (have_sme)
+		orig_tpidr2 = get_tpidr2();
+
+	if (!get_current_context(td, &context.uc, sizeof(context)))
+		return 1;
+
+	tpidr2_ctx = (struct tpidr2_context *)
+		get_header(head, TPIDR2_MAGIC, td->live_sz, &offset);
+
+	in_sigframe = tpidr2_ctx != NULL;
+
+	fprintf(stderr, "TPIDR2 sigframe %s on system %s SME\n",
+		in_sigframe ? "present" : "absent",
+		have_sme ? "with" : "without");
+
+	td->pass = (in_sigframe == have_sme);
+
+	/*
+	 * Check that the value we read back was the one present at
+	 * the time that the signal was triggered.  TPIDR2 is owned by
+	 * libc so we can't safely choose the value and it is possible
+	 * that we may need to revisit this in future if something
+	 * starts deciding to set a new TPIDR2 between us reading and
+	 * the signal.
+	 */
+	if (have_sme && tpidr2_ctx) {
+		if (tpidr2_ctx->tpidr2 != orig_tpidr2) {
+			fprintf(stderr, "TPIDR2 in frame is %llx, was %llx\n",
+				tpidr2_ctx->tpidr2, orig_tpidr2);
+			td->pass = false;
+		}
+	}
+
+	return 0;
+}
+
+struct tdescr tde = {
+	.name = "TPIDR2",
+	.descr = "Validate that TPIDR2 is present as expected",
+	.timeout = 3,
+	.run = tpidr2_present,
+};
-- 
2.30.2


_______________________________________________
linux-arm-kernel mailing list
linux-arm-kernel@lists.infradead.org
http://lists.infradead.org/mailman/listinfo/linux-arm-kernel

Re: [PATCH v2 2/4] arm64/signal: Include TPIDR2 in the signal context

[Will Deacon]

On Mon, Oct 31, 2022 at 08:17:34PM +0000, Mark Brown wrote:
> Add a new signal frame record for TPIDR2 using the same format as we
> already use for ESR with different magic, a header with the value from the
> register appended as the only data. If SME is supported then this record is
> always included.
> 
> Signed-off-by: Mark Brown <broonie@kernel.org>
> Reviewed-by: Szabolcs Nagy <szabolcs.nagy@arm.com>
> ---
>  arch/arm64/include/uapi/asm/sigcontext.h |  8 ++++
>  arch/arm64/kernel/signal.c               | 59 ++++++++++++++++++++++++
>  2 files changed, 67 insertions(+)
> 
> diff --git a/arch/arm64/include/uapi/asm/sigcontext.h b/arch/arm64/include/uapi/asm/sigcontext.h
> index 4aaf31e3bf16..46ce097ca8c0 100644
> --- a/arch/arm64/include/uapi/asm/sigcontext.h
> +++ b/arch/arm64/include/uapi/asm/sigcontext.h
> @@ -140,6 +140,14 @@ struct sve_context {
>  
>  #define SVE_SIG_FLAG_SM	0x1	/* Context describes streaming mode */
>  
> +/* TPIDR2_EL0 context */
> +#define TPIDR2_MAGIC	0x54504902
> +
> +struct tpidr2_context {
> +	struct _aarch64_ctx head;
> +	__u64 tpidr2;
> +};
> +
>  #define ZA_MAGIC	0x54366345
>  
>  struct za_context {
> diff --git a/arch/arm64/kernel/signal.c b/arch/arm64/kernel/signal.c
> index 9ad911f1647c..ac4fb42a9613 100644
> --- a/arch/arm64/kernel/signal.c
> +++ b/arch/arm64/kernel/signal.c
> @@ -56,6 +56,7 @@ struct rt_sigframe_user_layout {
>  	unsigned long fpsimd_offset;
>  	unsigned long esr_offset;
>  	unsigned long sve_offset;
> +	unsigned long tpidr2_offset;
>  	unsigned long za_offset;
>  	unsigned long extra_offset;
>  	unsigned long end_offset;
> @@ -219,6 +220,7 @@ static int restore_fpsimd_context(struct fpsimd_context __user *ctx)
>  struct user_ctxs {
>  	struct fpsimd_context __user *fpsimd;
>  	struct sve_context __user *sve;
> +	struct tpidr2_context __user *tpidr2;
>  	struct za_context __user *za;
>  };
>  
> @@ -358,6 +360,32 @@ extern int preserve_sve_context(void __user *ctx);
>  
>  #ifdef CONFIG_ARM64_SME
>  
> +static int preserve_tpidr2_context(struct tpidr2_context __user *ctx)
> +{
> +	int err = 0;
> +
> +	current->thread.tpidr2_el0 = read_sysreg_s(SYS_TPIDR2_EL0);

This is half of tls_preserve_current_state() and may be worth factoring out
so that ptrace and signal delivery can use the same sets of helpers.

> +
> +	__put_user_error(TPIDR2_MAGIC, &ctx->head.magic, err);
> +	__put_user_error(sizeof(*ctx), &ctx->head.size, err);
> +	__put_user_error(current->thread.tpidr2_el0, &ctx->tpidr2, err);
> +
> +	return err;
> +}
> +
> +static int restore_tpidr2_context(struct user_ctxs *user)
> +{
> +	u64 tpidr2_el0;
> +	int err = 0;
> +
> +	/* Magic and size were validated deciding to call this function */

typo: before deciding

> +	__get_user_error(tpidr2_el0, &user->tpidr2->tpidr2, err);
> +	if (!err)
> +		current->thread.tpidr2_el0 = tpidr2_el0;

What guarantees this makes its way into the hardware register before we
return to userspace, context switch or deliver another signal?

> +
> +	return err;
> +}
> +
>  static int preserve_za_context(struct za_context __user *ctx)
>  {
>  	int err = 0;
> @@ -447,6 +475,8 @@ static int restore_za_context(struct user_ctxs *user)
>  #else /* ! CONFIG_ARM64_SME */
>  
>  /* Turn any non-optimised out attempts to use these into a link error: */
> +extern int preserve_tpidr2_context(void __user *ctx);
> +extern int restore_tpidr2_context(struct user_ctxs *user);
>  extern int preserve_za_context(void __user *ctx);
>  extern int restore_za_context(struct user_ctxs *user);
>  
> @@ -465,6 +495,7 @@ static int parse_user_sigframe(struct user_ctxs *user,
>  
>  	user->fpsimd = NULL;
>  	user->sve = NULL;
> +	user->tpidr2 = NULL;
>  	user->za = NULL;
>  
>  	if (!IS_ALIGNED((unsigned long)base, 16))
> @@ -531,6 +562,19 @@ static int parse_user_sigframe(struct user_ctxs *user,
>  			user->sve = (struct sve_context __user *)head;
>  			break;
>  
> +		case TPIDR2_MAGIC:
> +			if (!system_supports_sme())

We seem to be inconsistent between using system_supports_sme() and
system_supports_tpidr2() for tpidr2 checks (see tls_get() and tls_set() for
a glaring example). Please can you tidy this up as a preliminary patch?

> +				goto invalid;
> +
> +			if (user->tpidr2)
> +				goto invalid;
> +
> +			if (size != sizeof(*user->tpidr2))

Why are you requiring an exact match here? Won't that hinder any future
extension of the structure?

Will

_______________________________________________
linux-arm-kernel mailing list
linux-arm-kernel@lists.infradead.org
http://lists.infradead.org/mailman/listinfo/linux-arm-kernel

Re: [PATCH v2 2/4] arm64/signal: Include TPIDR2 in the signal context

[Mark Brown]

[-- Attachment #1.1: Type: text/plain, Size: 642 bytes --]

On Mon, Nov 14, 2022 at 04:10:06PM +0000, Will Deacon wrote:
> On Mon, Oct 31, 2022 at 08:17:34PM +0000, Mark Brown wrote:

> > +			if (user->tpidr2)
> > +				goto invalid;

> > +			if (size != sizeof(*user->tpidr2))

> Why are you requiring an exact match here? Won't that hinder any future
> extension of the structure?

It will but since the structure is explicitly for a single sysreg
that's intentional - the thinking was to just continue to model
any more sysregs we want to report in the signal context in the
same format with their own contexts.  It felt like it fit better
into how everything else in the signal context is extended.

[-- Attachment #1.2: signature.asc --]
[-- Type: application/pgp-signature, Size: 488 bytes --]

[-- Attachment #2: Type: text/plain, Size: 176 bytes --]

_______________________________________________
linux-arm-kernel mailing list
linux-arm-kernel@lists.infradead.org
http://lists.infradead.org/mailman/listinfo/linux-arm-kernel

Re: [PATCH v2 2/4] arm64/signal: Include TPIDR2 in the signal context

[Will Deacon]

On Tue, Nov 15, 2022 at 12:08:44PM +0000, Mark Brown wrote:
> On Mon, Nov 14, 2022 at 04:10:06PM +0000, Will Deacon wrote:
> > On Mon, Oct 31, 2022 at 08:17:34PM +0000, Mark Brown wrote:
> 
> > > +			if (user->tpidr2)
> > > +				goto invalid;
> 
> > > +			if (size != sizeof(*user->tpidr2))
> 
> > Why are you requiring an exact match here? Won't that hinder any future
> > extension of the structure?
> 
> It will but since the structure is explicitly for a single sysreg
> that's intentional - the thinking was to just continue to model
> any more sysregs we want to report in the signal context in the
> same format with their own contexts.  It felt like it fit better
> into how everything else in the signal context is extended.

I see, but having the usual '<' check wouldn't preclude us from doing
what you suggest above, whilst also giving us some flexibility in case
things turn out differently from how we expected.

Will

_______________________________________________
linux-arm-kernel mailing list
linux-arm-kernel@lists.infradead.org
http://lists.infradead.org/mailman/listinfo/linux-arm-kernel

Re: [PATCH v2 2/4] arm64/signal: Include TPIDR2 in the signal context

[Mark Brown]

[-- Attachment #1.1: Type: text/plain, Size: 1802 bytes --]

On Tue, Nov 15, 2022 at 12:22:26PM +0000, Will Deacon wrote:
> On Tue, Nov 15, 2022 at 12:08:44PM +0000, Mark Brown wrote:
> > On Mon, Nov 14, 2022 at 04:10:06PM +0000, Will Deacon wrote:
> > > On Mon, Oct 31, 2022 at 08:17:34PM +0000, Mark Brown wrote:

> > > > +			if (user->tpidr2)
> > > > +				goto invalid;

> > > > +			if (size != sizeof(*user->tpidr2))

> > > Why are you requiring an exact match here? Won't that hinder any future
> > > extension of the structure?

> > It will but since the structure is explicitly for a single sysreg
> > that's intentional - the thinking was to just continue to model
> > any more sysregs we want to report in the signal context in the
> > same format with their own contexts.  It felt like it fit better
> > into how everything else in the signal context is extended.

> I see, but having the usual '<' check wouldn't preclude us from doing
> what you suggest above, whilst also giving us some flexibility in case
> things turn out differently from how we expected.

This actually also how we validate the base fpsimd_context -
while there is a < check in the switch statement in
parse_user_sigframe() but we also have an exact size check
near the top of restore_fpsimd_context() which gets called from
there, meaning that the check in parse_user_sigframe() is a bit
redundant.  We do however allow the varibly sized frames to have
an oversized allocation, though those have internal sizing
information whereas fpsimd_context doesn't.  My take was that we
were erroring out here because if userspace thinks it's supplying
some state that we're ignoring and not restoring then things
might go badly.  I'm not super wedded to this approach but it is
consistent with the fpsimd_context handling and I can see some
justificaton for it being done the way it is.

[-- Attachment #1.2: signature.asc --]
[-- Type: application/pgp-signature, Size: 488 bytes --]

[-- Attachment #2: Type: text/plain, Size: 176 bytes --]

_______________________________________________
linux-arm-kernel mailing list
linux-arm-kernel@lists.infradead.org
http://lists.infradead.org/mailman/listinfo/linux-arm-kernel

Re: [PATCH v2 2/4] arm64/signal: Include TPIDR2 in the signal context

[Mark Brown]

[-- Attachment #1.1: Type: text/plain, Size: 649 bytes --]

On Mon, Nov 14, 2022 at 04:10:06PM +0000, Will Deacon wrote:
> On Mon, Oct 31, 2022 at 08:17:34PM +0000, Mark Brown wrote:

> > +	__get_user_error(tpidr2_el0, &user->tpidr2->tpidr2, err);
> > +	if (!err)
> > +		current->thread.tpidr2_el0 = tpidr2_el0;

> What guarantees this makes its way into the hardware register before we
> return to userspace, context switch or deliver another signal?

Context switch is handled, part of context switch is to restore
the value from the task struct, but other cases aren't AFAICT.
This is in general an oversight in our signal testing framework,
it does not have a pattern of validating values set in signals.

[-- Attachment #1.2: signature.asc --]
[-- Type: application/pgp-signature, Size: 488 bytes --]

[-- Attachment #2: Type: text/plain, Size: 176 bytes --]

_______________________________________________
linux-arm-kernel mailing list
linux-arm-kernel@lists.infradead.org
http://lists.infradead.org/mailman/listinfo/linux-arm-kernel

Re: [PATCH v2 2/4] arm64/signal: Include TPIDR2 in the signal context

[Will Deacon]

On Tue, Nov 15, 2022 at 05:13:39PM +0000, Mark Brown wrote:
> On Mon, Nov 14, 2022 at 04:10:06PM +0000, Will Deacon wrote:
> > On Mon, Oct 31, 2022 at 08:17:34PM +0000, Mark Brown wrote:
> 
> > > +	__get_user_error(tpidr2_el0, &user->tpidr2->tpidr2, err);
> > > +	if (!err)
> > > +		current->thread.tpidr2_el0 = tpidr2_el0;
> 
> > What guarantees this makes its way into the hardware register before we
> > return to userspace, context switch or deliver another signal?
> 
> Context switch is handled, part of context switch is to restore
> the value from the task struct, but other cases aren't AFAICT.
> This is in general an oversight in our signal testing framework,
> it does not have a pattern of validating values set in signals.

Right, but my question is about going to userspace _before_ we context
switch, so I think there's an issue here.

Will

_______________________________________________
linux-arm-kernel mailing list
linux-arm-kernel@lists.infradead.org
http://lists.infradead.org/mailman/listinfo/linux-arm-kernel

Re: [PATCH v2 2/4] arm64/signal: Include TPIDR2 in the signal context

[Will Deacon]

On Tue, Nov 15, 2022 at 04:42:14PM +0000, Mark Brown wrote:
> On Tue, Nov 15, 2022 at 12:22:26PM +0000, Will Deacon wrote:
> > On Tue, Nov 15, 2022 at 12:08:44PM +0000, Mark Brown wrote:
> > > On Mon, Nov 14, 2022 at 04:10:06PM +0000, Will Deacon wrote:
> > > > On Mon, Oct 31, 2022 at 08:17:34PM +0000, Mark Brown wrote:
> 
> > > > > +			if (user->tpidr2)
> > > > > +				goto invalid;
> 
> > > > > +			if (size != sizeof(*user->tpidr2))
> 
> > > > Why are you requiring an exact match here? Won't that hinder any future
> > > > extension of the structure?
> 
> > > It will but since the structure is explicitly for a single sysreg
> > > that's intentional - the thinking was to just continue to model
> > > any more sysregs we want to report in the signal context in the
> > > same format with their own contexts.  It felt like it fit better
> > > into how everything else in the signal context is extended.
> 
> > I see, but having the usual '<' check wouldn't preclude us from doing
> > what you suggest above, whilst also giving us some flexibility in case
> > things turn out differently from how we expected.
> 
> This actually also how we validate the base fpsimd_context -
> while there is a < check in the switch statement in
> parse_user_sigframe() but we also have an exact size check
> near the top of restore_fpsimd_context() which gets called from
> there, meaning that the check in parse_user_sigframe() is a bit
> redundant.  We do however allow the varibly sized frames to have
> an oversized allocation, though those have internal sizing
> information whereas fpsimd_context doesn't.  My take was that we
> were erroring out here because if userspace thinks it's supplying
> some state that we're ignoring and not restoring then things
> might go badly.  I'm not super wedded to this approach but it is
> consistent with the fpsimd_context handling and I can see some
> justificaton for it being done the way it is.

Hmm, good point about fpsimd, it looks at magic/size twice which is
definitely wrong (userspace could even change those values in between!).

So I'd vote for removing the checks from restore_fpsimd_context() which
raises the same question we were discussing initially: should the check
in parse_user_sigframe() require an exact size match or instead truncate
the structure on the stack by only copying a prefix into the kernel?

I'm actually warming more towards an exact check now that we've spoken
about it a bit... What do you think?

Will

_______________________________________________
linux-arm-kernel mailing list
linux-arm-kernel@lists.infradead.org
http://lists.infradead.org/mailman/listinfo/linux-arm-kernel

Re: [PATCH v2 2/4] arm64/signal: Include TPIDR2 in the signal context

[Mark Brown]

[-- Attachment #1.1: Type: text/plain, Size: 561 bytes --]

On Fri, Nov 18, 2022 at 01:53:43PM +0000, Will Deacon wrote:
> On Tue, Nov 15, 2022 at 05:13:39PM +0000, Mark Brown wrote:

> > Context switch is handled, part of context switch is to restore
> > the value from the task struct, but other cases aren't AFAICT.
> > This is in general an oversight in our signal testing framework,
> > it does not have a pattern of validating values set in signals.

> Right, but my question is about going to userspace _before_ we context
> switch, so I think there's an issue here.

Yes - like I say "other cases aren't AFAICT".

[-- Attachment #1.2: signature.asc --]
[-- Type: application/pgp-signature, Size: 488 bytes --]

[-- Attachment #2: Type: text/plain, Size: 176 bytes --]

_______________________________________________
linux-arm-kernel mailing list
linux-arm-kernel@lists.infradead.org
http://lists.infradead.org/mailman/listinfo/linux-arm-kernel

Re: [PATCH v2 2/4] arm64/signal: Include TPIDR2 in the signal context

[Mark Brown]

[-- Attachment #1.1: Type: text/plain, Size: 2153 bytes --]

On Fri, Nov 18, 2022 at 01:55:53PM +0000, Will Deacon wrote:
> On Tue, Nov 15, 2022 at 04:42:14PM +0000, Mark Brown wrote:

> > > I see, but having the usual '<' check wouldn't preclude us from doing
> > > what you suggest above, whilst also giving us some flexibility in case
> > > things turn out differently from how we expected.

> > This actually also how we validate the base fpsimd_context -
> > while there is a < check in the switch statement in
> > parse_user_sigframe() but we also have an exact size check
> > near the top of restore_fpsimd_context() which gets called from

> Hmm, good point about fpsimd, it looks at magic/size twice which is
> definitely wrong (userspace could even change those values in between!).

> So I'd vote for removing the checks from restore_fpsimd_context() which
> raises the same question we were discussing initially: should the check
> in parse_user_sigframe() require an exact size match or instead truncate
> the structure on the stack by only copying a prefix into the kernel?

I think it might be best to go the other way and remove the size
checks from parse_user_sigframe() entirely and put them into the
frame type specific functions so that parse_user_sigframe() looks
the same for all frame types.  We should make sure that the
individual frame type functions don't re-read the magic and size
though, only reading the additional frame type specific data and
doing whatever validation makes sense.

> I'm actually warming more towards an exact check now that we've spoken
> about it a bit... What do you think?

I'm more inclined towards exact checks, it seems safer.  It's
hard to see how we'd use any extensibility in a way that doesn't
go bad in some situation or other if the frame isn't designed to
be variably sized from the start, it's a lot easier to add a new
frame safely than to extend an existing one.

I'll send a new version fixing all the other issues you pointed
out and with size the check code as is (apart from anything else
I already prepared it) and then look at fixing up the behaviour
for FPSIMD and generally around parse_user_sigframe() incrementally.
Hopefully that's OK.

[-- Attachment #1.2: signature.asc --]
[-- Type: application/pgp-signature, Size: 488 bytes --]

[-- Attachment #2: Type: text/plain, Size: 176 bytes --]

_______________________________________________
linux-arm-kernel mailing list
linux-arm-kernel@lists.infradead.org
http://lists.infradead.org/mailman/listinfo/linux-arm-kernel

Re: [PATCH v2 2/4] arm64/signal: Include TPIDR2 in the signal context

[Catalin Marinas]

On Fri, Nov 18, 2022 at 01:55:53PM +0000, Will Deacon wrote:
> On Tue, Nov 15, 2022 at 04:42:14PM +0000, Mark Brown wrote:
> > On Tue, Nov 15, 2022 at 12:22:26PM +0000, Will Deacon wrote:
> > > On Tue, Nov 15, 2022 at 12:08:44PM +0000, Mark Brown wrote:
> > > > On Mon, Nov 14, 2022 at 04:10:06PM +0000, Will Deacon wrote:
> > > > > On Mon, Oct 31, 2022 at 08:17:34PM +0000, Mark Brown wrote:
> > 
> > > > > > +			if (user->tpidr2)
> > > > > > +				goto invalid;
> > 
> > > > > > +			if (size != sizeof(*user->tpidr2))
> > 
> > > > > Why are you requiring an exact match here? Won't that hinder any future
> > > > > extension of the structure?
> > 
> > > > It will but since the structure is explicitly for a single sysreg
> > > > that's intentional - the thinking was to just continue to model
> > > > any more sysregs we want to report in the signal context in the
> > > > same format with their own contexts.  It felt like it fit better
> > > > into how everything else in the signal context is extended.
> > 
> > > I see, but having the usual '<' check wouldn't preclude us from doing
> > > what you suggest above, whilst also giving us some flexibility in case
> > > things turn out differently from how we expected.
> > 
> > This actually also how we validate the base fpsimd_context -
> > while there is a < check in the switch statement in
> > parse_user_sigframe() but we also have an exact size check
> > near the top of restore_fpsimd_context() which gets called from
> > there, meaning that the check in parse_user_sigframe() is a bit
> > redundant.  We do however allow the varibly sized frames to have
> > an oversized allocation, though those have internal sizing
> > information whereas fpsimd_context doesn't.  My take was that we
> > were erroring out here because if userspace thinks it's supplying
> > some state that we're ignoring and not restoring then things
> > might go badly.  I'm not super wedded to this approach but it is
> > consistent with the fpsimd_context handling and I can see some
> > justificaton for it being done the way it is.
> 
> Hmm, good point about fpsimd, it looks at magic/size twice which is
> definitely wrong (userspace could even change those values in between!).
> 
> So I'd vote for removing the checks from restore_fpsimd_context() which
> raises the same question we were discussing initially: should the check
> in parse_user_sigframe() require an exact size match or instead truncate
> the structure on the stack by only copying a prefix into the kernel?
> 
> I'm actually warming more towards an exact check now that we've spoken
> about it a bit... What do you think?

I'd go for an exact match as well. I don't think we can expand these
structures in the future safely without an additional magic number.

I tend to agree with Mark here that parse_user_sigframe() should only
check the magic numbers and set the corresponding user_ctxs members. We
leave the exact size check to the restore_fpsimd_context() etc. (can
skip the magic check here). Well, not a strong view either way but we
should definitely remove the duplicate checks.

-- 
Catalin

_______________________________________________
linux-arm-kernel mailing list
linux-arm-kernel@lists.infradead.org
http://lists.infradead.org/mailman/listinfo/linux-arm-kernel

Re: [PATCH v2 2/4] arm64/signal: Include TPIDR2 in the signal context

[Mark Brown]

[-- Attachment #1.1: Type: text/plain, Size: 1316 bytes --]

On Fri, Nov 18, 2022 at 07:10:16PM +0000, Catalin Marinas wrote:
> On Fri, Nov 18, 2022 at 01:55:53PM +0000, Will Deacon wrote:

> > So I'd vote for removing the checks from restore_fpsimd_context() which
> > raises the same question we were discussing initially: should the check
> > in parse_user_sigframe() require an exact size match or instead truncate
> > the structure on the stack by only copying a prefix into the kernel?

> > I'm actually warming more towards an exact check now that we've spoken
> > about it a bit... What do you think?

> I'd go for an exact match as well. I don't think we can expand these
> structures in the future safely without an additional magic number.

> I tend to agree with Mark here that parse_user_sigframe() should only
> check the magic numbers and set the corresponding user_ctxs members. We
> leave the exact size check to the restore_fpsimd_context() etc. (can
> skip the magic check here). Well, not a strong view either way but we
> should definitely remove the duplicate checks.

I'm working on this, should have something out in the next day or so,
but it's getting a bit more involved than just a simple fix (the main
thing is that the various frame type parsers aren't consistent in how
they're called which makes things a bit more invasive than they should
be).

[-- Attachment #1.2: signature.asc --]
[-- Type: application/pgp-signature, Size: 488 bytes --]

[-- Attachment #2: Type: text/plain, Size: 176 bytes --]

_______________________________________________
linux-arm-kernel mailing list
linux-arm-kernel@lists.infradead.org
http://lists.infradead.org/mailman/listinfo/linux-arm-kernel