[PATCH 0/5] arm64: signal: Signal frame expansion support

[PATCH 0/5] arm64: signal: Signal frame expansion support

[Dave Martin]

This series supersedes a prior RFC [1].

This series refactors the arm64 signal handling implementation in order
to make it easier to add support for new CPU architecture extensions,
which may require new records in the signal frame and may require the
signal frame to grow beyond its current size.

This series provides infrastructure that will be needed in order to
support the Scalable Vector Extension (SVE) [2] (patches to be posted
separately).

LTP testing is ongoing -- I will ping this series when I have results.


Changes since RFC v2:

 * The frame link record has been moved back to the end of the full
   signal frame, instead of fixing its position at the end of
   __reserved[], after Catalin raised concerns about the space otherwise
   wasted by putting the extended signal frame data after it.

   Previous versions of the series had this behaviour, but concerns were
   expressed by reviewers about the potential ABI impact.

   This change has been informally tested for ABI impacts using the
   libgcc unwinder (exercised by throwing exceptions from a signal
   handler with g++ -fnon-call-exceptions) and gdb backtracing, neither
   of which showed any problem.  Review of the libgcc and libunwind
   implementations and discussion with gdb developers suggest that no
   breakage is expected as a result of the change.

   (See patch 1 of this posting.)

 * Patches 4 (Allocate extra sigcontext space as needed) and 5 (Parse
   extra_context during sigreturn) of RFC v2 have been merged, since
   they are not fully bisectable and make more sense as a single patch.

 * Patch 6 (Report signal frame size to userspace via auxv) of RFC v2
   has been dropped from the series, because this creates ABI that we
   don't need yet, and can benefit from further discussion.  This patch
   will become relevant when merging support for SVE.

 * struct extra_context is explicitly padded up to a multiple of 16 bytes.
   This fixes a bug whereby a non-extra_context-aware unwinder would
   skip the wrong amount of data after extra_context and miss the
   terminator record.

   There is a de facto assumption that unwinders do not need to round
   _aarch64_ctx.size up to a multiple of 16 when stepping through the
   records in the signal frame, though this isn't documented anywhere.


Original blurb:

An architecture advertises the maximum possible signal frame size via
the MINSIGSTKSZ #define (mandated by POSIX).

However, CPU architecture extensions may increase the amount of space
required to store the interrupted context when a signal is delivered.


Eventually the amount of space needed in the signal frame may exceed
MINSIGSTKSZ -- whether and when this happens is largely a matter of
luck, depending on the initial guess for MINSIGSTKSZ and the evolution
of that particular CPU architecture.  Unfortunately MINSIGSTKSZ cannot
be changed without an ABI break, and POSIX provides no mechanism for
migration.

arm64 initially reserved 4KB of space in the signal frame for
extensions, of which about 0.5KB is allocated to the FP/SIMD registers
initially.

Depending on the vector length supported by the hardware, SVE requires
up to around 8KB of space to store the full SIMD register context, which
is too large to fit in the existing frame.

This series adds a mechanism for optionally enlarging the signal frame
(patches 4-5) and reporting the actual maximum signal frame size to
userspace (patch 6).  Patches 1-3 do some refactoring to support this
change by abstracting the way signal frame records are allocated onto
the user stack.

Full backwards compatibility is not possible -- there is no way to hide
the fact that the signal frame has grown -- so it is expected that
support for new architecture extensions that can cause the signal frame
to grow will be opt-in for userspace, in addition to using the extension
mechanism defined by this series.


[1] [RFC PATCH v2 0/6] Signal frame expansion support
http://lists.infradead.org/pipermail/linux-arm-kernel/2017-April/501163.html

[2] ARM Scalable Vector Extension
https://community.arm.com/groups/processors/blog/2016/08/22/technology-update-the-scalable-vector-extension-sve-for-the-armv8-a-architecture
https://developer.arm.com/docs/ddi0584/latest/arm-architecture-reference-manual-supplement-the-scalable-vector-extension-sve-for-armv8-a


Dave Martin (5):
  arm64: signal: split frame link record from sigcontext structure
  arm64: signal: Refactor sigcontext parsing in rt_sigreturn
  arm64: signal: factor frame layout and population into separate passes
  arm64: signal: factor out signal frame record allocation
  arm64: signal: Allow expansion of the signal frame

 arch/arm64/include/uapi/asm/sigcontext.h |  55 +++++
 arch/arm64/kernel/signal.c               | 408 ++++++++++++++++++++++++++++---
 2 files changed, 427 insertions(+), 36 deletions(-)

-- 
2.1.4

[PATCH 1/5] arm64: signal: split frame link record from sigcontext structure

[Dave Martin]

In order to be able to increase the amount of the data currently
written to the __reserved[] array in the signal frame, it is
necessary to overwrite the locations currently occupied by the
{fp,lr} frame link record pushed at the top of the signal stack.

In order for this to work, this patch detaches the frame link
record from struct rt_sigframe and places it separately at the top
of the signal stack.  This will allow subsequent patches to insert
data between it and __reserved[].

This change relies on the non-ABI status of the placement of the
frame record with respect to struct sigframe: this status is
undocumented, but the placement is not declared or described in the
user headers, and known unwinder implementations (libgcc,
libunwind, gdb) appear not to rely on it.

Signed-off-by: Dave Martin <Dave.Martin@arm.com>
---
 arch/arm64/kernel/signal.c | 50 +++++++++++++++++++++++++++++-----------------
 1 file changed, 32 insertions(+), 18 deletions(-)

diff --git a/arch/arm64/kernel/signal.c b/arch/arm64/kernel/signal.c
index c7b6de6..1e5ed3be 100644
--- a/arch/arm64/kernel/signal.c
+++ b/arch/arm64/kernel/signal.c
@@ -19,6 +19,7 @@
 
 #include <linux/compat.h>
 #include <linux/errno.h>
+#include <linux/kernel.h>
 #include <linux/signal.h>
 #include <linux/personality.h>
 #include <linux/freezer.h>
@@ -41,10 +42,18 @@
 struct rt_sigframe {
 	struct siginfo info;
 	struct ucontext uc;
+};
+
+struct frame_record {
 	u64 fp;
 	u64 lr;
 };
 
+struct rt_sigframe_user_layout {
+	struct rt_sigframe __user *sigframe;
+	struct frame_record __user *next_frame;
+};
+
 static int preserve_fpsimd_context(struct fpsimd_context __user *ctx)
 {
 	struct fpsimd_state *fpsimd = &current->thread.fpsimd_state;
@@ -162,16 +171,17 @@ asmlinkage long sys_rt_sigreturn(struct pt_regs *regs)
 	return 0;
 }
 
-static int setup_sigframe(struct rt_sigframe __user *sf,
+static int setup_sigframe(struct rt_sigframe_user_layout *user,
 			  struct pt_regs *regs, sigset_t *set)
 {
 	int i, err = 0;
+	struct rt_sigframe __user *sf = user->sigframe;
 	void *aux = sf->uc.uc_mcontext.__reserved;
 	struct _aarch64_ctx *end;
 
 	/* set up the stack frame for unwinding */
-	__put_user_error(regs->regs[29], &sf->fp, err);
-	__put_user_error(regs->regs[30], &sf->lr, err);
+	__put_user_error(regs->regs[29], &user->next_frame->fp, err);
+	__put_user_error(regs->regs[30], &user->next_frame->lr, err);
 
 	for (i = 0; i < 31; i++)
 		__put_user_error(regs->regs[i], &sf->uc.uc_mcontext.regs[i],
@@ -209,34 +219,36 @@ static int setup_sigframe(struct rt_sigframe __user *sf,
 	return err;
 }
 
-static struct rt_sigframe __user *get_sigframe(struct ksignal *ksig,
-					       struct pt_regs *regs)
+static int get_sigframe(struct rt_sigframe_user_layout *user,
+			 struct ksignal *ksig, struct pt_regs *regs)
 {
 	unsigned long sp, sp_top;
-	struct rt_sigframe __user *frame;
 
 	sp = sp_top = sigsp(regs->sp, ksig);
 
-	sp = (sp - sizeof(struct rt_sigframe)) & ~15;
-	frame = (struct rt_sigframe __user *)sp;
+	sp = round_down(sp - sizeof(struct frame_record), 16);
+	user->next_frame = (struct frame_record __user *)sp;
+
+	sp = round_down(sp - sizeof(struct rt_sigframe), 16);
+	user->sigframe = (struct rt_sigframe __user *)sp;
 
 	/*
 	 * Check that we can actually write to the signal frame.
 	 */
-	if (!access_ok(VERIFY_WRITE, frame, sp_top - sp))
-		frame = NULL;
+	if (!access_ok(VERIFY_WRITE, user->sigframe, sp_top - sp))
+		return -EFAULT;
 
-	return frame;
+	return 0;
 }
 
 static void setup_return(struct pt_regs *regs, struct k_sigaction *ka,
-			 void __user *frame, int usig)
+			 struct rt_sigframe_user_layout *user, int usig)
 {
 	__sigrestore_t sigtramp;
 
 	regs->regs[0] = usig;
-	regs->sp = (unsigned long)frame;
-	regs->regs[29] = regs->sp + offsetof(struct rt_sigframe, fp);
+	regs->sp = (unsigned long)user->sigframe;
+	regs->regs[29] = (unsigned long)&user->next_frame->fp;
 	regs->pc = (unsigned long)ka->sa.sa_handler;
 
 	if (ka->sa.sa_flags & SA_RESTORER)
@@ -250,20 +262,22 @@ static void setup_return(struct pt_regs *regs, struct k_sigaction *ka,
 static int setup_rt_frame(int usig, struct ksignal *ksig, sigset_t *set,
 			  struct pt_regs *regs)
 {
+	struct rt_sigframe_user_layout user;
 	struct rt_sigframe __user *frame;
 	int err = 0;
 
-	frame = get_sigframe(ksig, regs);
-	if (!frame)
+	if (get_sigframe(&user, ksig, regs))
 		return 1;
 
+	frame = user.sigframe;
+
 	__put_user_error(0, &frame->uc.uc_flags, err);
 	__put_user_error(NULL, &frame->uc.uc_link, err);
 
 	err |= __save_altstack(&frame->uc.uc_stack, regs->sp);
-	err |= setup_sigframe(frame, regs, set);
+	err |= setup_sigframe(&user, regs, set);
 	if (err == 0) {
-		setup_return(regs, &ksig->ka, frame, usig);
+		setup_return(regs, &ksig->ka, &user, usig);
 		if (ksig->ka.sa.sa_flags & SA_SIGINFO) {
 			err |= copy_siginfo_to_user(&frame->info, &ksig->info);
 			regs->regs[1] = (unsigned long)&frame->info;
-- 
2.1.4

[PATCH 2/5] arm64: signal: Refactor sigcontext parsing in rt_sigreturn

[Dave Martin]

Currently, rt_sigreturn does very limited checking on the
sigcontext coming from userspace.

Future additions to the sigcontext data will increase the potential
for surprises.  Also, it is not clear whether the sigcontext
extension records are supposed to occur in a particular order.

To allow the parsing code to be extended more easily, this patch
factors out the sigcontext parsing into a separate function, and
adds extra checks to validate the well-formedness of the sigcontext
structure.

Signed-off-by: Dave Martin <Dave.Martin@arm.com>
---
 arch/arm64/kernel/signal.c | 86 ++++++++++++++++++++++++++++++++++++++++++----
 1 file changed, 80 insertions(+), 6 deletions(-)

diff --git a/arch/arm64/kernel/signal.c b/arch/arm64/kernel/signal.c
index 1e5ed3be..67769f6 100644
--- a/arch/arm64/kernel/signal.c
+++ b/arch/arm64/kernel/signal.c
@@ -23,6 +23,7 @@
 #include <linux/signal.h>
 #include <linux/personality.h>
 #include <linux/freezer.h>
+#include <linux/stddef.h>
 #include <linux/uaccess.h>
 #include <linux/tracehook.h>
 #include <linux/ratelimit.h>
@@ -101,12 +102,86 @@ static int restore_fpsimd_context(struct fpsimd_context __user *ctx)
 	return err ? -EFAULT : 0;
 }
 
+struct user_ctxs {
+	struct fpsimd_context __user *fpsimd;
+};
+
+static int parse_user_sigframe(struct user_ctxs *user,
+			       struct rt_sigframe __user *sf)
+{
+	struct sigcontext __user *const sc = &sf->uc.uc_mcontext;
+	struct _aarch64_ctx __user *head =
+		(struct _aarch64_ctx __user *)&sc->__reserved;
+	size_t offset = 0;
+
+	user->fpsimd = NULL;
+
+	while (1) {
+		int err;
+		u32 magic, size;
+
+		head = (struct _aarch64_ctx __user *)&sc->__reserved[offset];
+		if (!IS_ALIGNED((unsigned long)head, 16))
+			goto invalid;
+
+		err = 0;
+		__get_user_error(magic, &head->magic, err);
+		__get_user_error(size, &head->size, err);
+		if (err)
+			return err;
+
+		switch (magic) {
+		case 0:
+			if (size)
+				goto invalid;
+
+			goto done;
+
+		case FPSIMD_MAGIC:
+			if (user->fpsimd)
+				goto invalid;
+
+			if (offset > sizeof(sc->__reserved) -
+					sizeof(*user->fpsimd) ||
+			    size < sizeof(*user->fpsimd))
+				goto invalid;
+
+			user->fpsimd = (struct fpsimd_context __user *)head;
+			break;
+
+		case ESR_MAGIC:
+			/* ignore */
+			break;
+
+		default:
+			goto invalid;
+		}
+
+		if (size < sizeof(*head))
+			goto invalid;
+
+		if (size > sizeof(sc->__reserved) - (sizeof(*head) + offset))
+			goto invalid;
+
+		offset += size;
+	}
+
+done:
+	if (!user->fpsimd)
+		goto invalid;
+
+	return 0;
+
+invalid:
+	return -EINVAL;
+}
+
 static int restore_sigframe(struct pt_regs *regs,
 			    struct rt_sigframe __user *sf)
 {
 	sigset_t set;
 	int i, err;
-	void *aux = sf->uc.uc_mcontext.__reserved;
+	struct user_ctxs user;
 
 	err = __copy_from_user(&set, &sf->uc.uc_sigmask, sizeof(set));
 	if (err == 0)
@@ -125,12 +200,11 @@ static int restore_sigframe(struct pt_regs *regs,
 	regs->syscallno = ~0UL;
 
 	err |= !valid_user_regs(&regs->user_regs, current);
+	if (err == 0)
+		err = parse_user_sigframe(&user, sf);
 
-	if (err == 0) {
-		struct fpsimd_context *fpsimd_ctx =
-			container_of(aux, struct fpsimd_context, head);
-		err |= restore_fpsimd_context(fpsimd_ctx);
-	}
+	if (err == 0)
+		err = restore_fpsimd_context(user.fpsimd);
 
 	return err;
 }
-- 
2.1.4

[PATCH 3/5] arm64: signal: factor frame layout and population into separate passes

[Dave Martin]

In preparation for expanding the signal frame, this patch refactors
the signal frame setup code in setup_sigframe() into two separate
passes.

The first pass, setup_sigframe_layout(), determines the size of the
signal frame and its internal layout, including the presence and
location of optional records.  The resulting knowledge is used to
allocate and locate the user stack space required for the signal
frame and to determine which optional records to include.

The second pass, setup_sigframe(), is called once the stack frame
is allocated in order to populate it with the necessary context
information.

As a result of these changes, it becomes more natural to represent
locations in the signal frame by a base pointer and an offset,
since the absolute address of each location is not known during the
layout pass.  To be more consistent with this logic,
parse_user_sigframe() is refactored to describe signal frame
locations in a similar way.

This change has no effect on the signal ABI, but will make it
easier to expand the signal frame in future patches.

Signed-off-by: Dave Martin <Dave.Martin@arm.com>
---
 arch/arm64/kernel/signal.c | 111 +++++++++++++++++++++++++++++++++++----------
 1 file changed, 88 insertions(+), 23 deletions(-)

diff --git a/arch/arm64/kernel/signal.c b/arch/arm64/kernel/signal.c
index 67769f6..eaef530 100644
--- a/arch/arm64/kernel/signal.c
+++ b/arch/arm64/kernel/signal.c
@@ -25,6 +25,7 @@
 #include <linux/freezer.h>
 #include <linux/stddef.h>
 #include <linux/uaccess.h>
+#include <linux/string.h>
 #include <linux/tracehook.h>
 #include <linux/ratelimit.h>
 
@@ -53,8 +54,39 @@ struct frame_record {
 struct rt_sigframe_user_layout {
 	struct rt_sigframe __user *sigframe;
 	struct frame_record __user *next_frame;
+
+	unsigned long size;	/* size of allocated sigframe data */
+	unsigned long limit;	/* largest allowed size */
+
+	unsigned long fpsimd_offset;
+	unsigned long esr_offset;
+	unsigned long end_offset;
 };
 
+static void init_user_layout(struct rt_sigframe_user_layout *user)
+{
+	memset(user, 0, sizeof(*user));
+	user->size = offsetof(struct rt_sigframe, uc.uc_mcontext.__reserved);
+
+	user->limit = user->size +
+		sizeof(user->sigframe->uc.uc_mcontext.__reserved) -
+		round_up(sizeof(struct _aarch64_ctx), 16);
+		/* ^ reserve space for terminator */
+}
+
+static size_t sigframe_size(struct rt_sigframe_user_layout const *user)
+{
+	return round_up(max(user->size, sizeof(struct rt_sigframe)), 16);
+}
+
+static void __user *apply_user_offset(
+	struct rt_sigframe_user_layout const *user, unsigned long offset)
+{
+	char __user *base = (char __user *)user->sigframe;
+
+	return base + offset;
+}
+
 static int preserve_fpsimd_context(struct fpsimd_context __user *ctx)
 {
 	struct fpsimd_state *fpsimd = &current->thread.fpsimd_state;
@@ -110,26 +142,35 @@ static int parse_user_sigframe(struct user_ctxs *user,
 			       struct rt_sigframe __user *sf)
 {
 	struct sigcontext __user *const sc = &sf->uc.uc_mcontext;
-	struct _aarch64_ctx __user *head =
-		(struct _aarch64_ctx __user *)&sc->__reserved;
+	struct _aarch64_ctx __user *head;
+	char __user *base = (char __user *)&sc->__reserved;
 	size_t offset = 0;
+	size_t limit = sizeof(sc->__reserved);
 
 	user->fpsimd = NULL;
 
+	if (!IS_ALIGNED((unsigned long)base, 16))
+		goto invalid;
+
 	while (1) {
-		int err;
+		int err = 0;
 		u32 magic, size;
 
-		head = (struct _aarch64_ctx __user *)&sc->__reserved[offset];
-		if (!IS_ALIGNED((unsigned long)head, 16))
+		if (limit - offset < sizeof(*head))
 			goto invalid;
 
-		err = 0;
+		if (!IS_ALIGNED(offset, 16))
+			goto invalid;
+
+		head = (struct _aarch64_ctx __user *)(base + offset);
 		__get_user_error(magic, &head->magic, err);
 		__get_user_error(size, &head->size, err);
 		if (err)
 			return err;
 
+		if (limit - offset < size)
+			goto invalid;
+
 		switch (magic) {
 		case 0:
 			if (size)
@@ -141,9 +182,7 @@ static int parse_user_sigframe(struct user_ctxs *user,
 			if (user->fpsimd)
 				goto invalid;
 
-			if (offset > sizeof(sc->__reserved) -
-					sizeof(*user->fpsimd) ||
-			    size < sizeof(*user->fpsimd))
+			if (size < sizeof(*user->fpsimd))
 				goto invalid;
 
 			user->fpsimd = (struct fpsimd_context __user *)head;
@@ -160,7 +199,7 @@ static int parse_user_sigframe(struct user_ctxs *user,
 		if (size < sizeof(*head))
 			goto invalid;
 
-		if (size > sizeof(sc->__reserved) - (sizeof(*head) + offset))
+		if (limit - offset < size)
 			goto invalid;
 
 		offset += size;
@@ -245,13 +284,30 @@ asmlinkage long sys_rt_sigreturn(struct pt_regs *regs)
 	return 0;
 }
 
+/* Determine the layout of optional records in the signal frame */
+static int setup_sigframe_layout(struct rt_sigframe_user_layout *user)
+{
+	user->fpsimd_offset = user->size;
+	user->size += round_up(sizeof(struct fpsimd_context), 16);
+
+	/* fault information, if valid */
+	if (current->thread.fault_code) {
+		user->esr_offset = user->size;
+		user->size += round_up(sizeof(struct esr_context), 16);
+	}
+
+	/* set the "end" magic */
+	user->end_offset = user->size;
+
+	return 0;
+}
+
+
 static int setup_sigframe(struct rt_sigframe_user_layout *user,
 			  struct pt_regs *regs, sigset_t *set)
 {
 	int i, err = 0;
 	struct rt_sigframe __user *sf = user->sigframe;
-	void *aux = sf->uc.uc_mcontext.__reserved;
-	struct _aarch64_ctx *end;
 
 	/* set up the stack frame for unwinding */
 	__put_user_error(regs->regs[29], &user->next_frame->fp, err);
@@ -269,26 +325,29 @@ static int setup_sigframe(struct rt_sigframe_user_layout *user,
 	err |= __copy_to_user(&sf->uc.uc_sigmask, set, sizeof(*set));
 
 	if (err == 0) {
-		struct fpsimd_context *fpsimd_ctx =
-			container_of(aux, struct fpsimd_context, head);
+		struct fpsimd_context __user *fpsimd_ctx =
+			apply_user_offset(user, user->fpsimd_offset);
 		err |= preserve_fpsimd_context(fpsimd_ctx);
-		aux += sizeof(*fpsimd_ctx);
 	}
 
 	/* fault information, if valid */
-	if (current->thread.fault_code) {
-		struct esr_context *esr_ctx =
-			container_of(aux, struct esr_context, head);
+	if (err == 0 && user->esr_offset) {
+		struct esr_context __user *esr_ctx =
+			apply_user_offset(user, user->esr_offset);
+
 		__put_user_error(ESR_MAGIC, &esr_ctx->head.magic, err);
 		__put_user_error(sizeof(*esr_ctx), &esr_ctx->head.size, err);
 		__put_user_error(current->thread.fault_code, &esr_ctx->esr, err);
-		aux += sizeof(*esr_ctx);
 	}
 
 	/* set the "end" magic */
-	end = aux;
-	__put_user_error(0, &end->magic, err);
-	__put_user_error(0, &end->size, err);
+	if (err == 0) {
+		struct _aarch64_ctx __user *end =
+			apply_user_offset(user, user->end_offset);
+
+		__put_user_error(0, &end->magic, err);
+		__put_user_error(0, &end->size, err);
+	}
 
 	return err;
 }
@@ -297,13 +356,19 @@ static int get_sigframe(struct rt_sigframe_user_layout *user,
 			 struct ksignal *ksig, struct pt_regs *regs)
 {
 	unsigned long sp, sp_top;
+	int err;
+
+	init_user_layout(user);
+	err = setup_sigframe_layout(user);
+	if (err)
+		return err;
 
 	sp = sp_top = sigsp(regs->sp, ksig);
 
 	sp = round_down(sp - sizeof(struct frame_record), 16);
 	user->next_frame = (struct frame_record __user *)sp;
 
-	sp = round_down(sp - sizeof(struct rt_sigframe), 16);
+	sp = round_down(sp, 16) - sigframe_size(user);
 	user->sigframe = (struct rt_sigframe __user *)sp;
 
 	/*
-- 
2.1.4

[PATCH 4/5] arm64: signal: factor out signal frame record allocation

[Dave Martin]

This patch factors out the allocator for signal frame optional
records into a separate function, to ensure consistency and
facilitate later expansion.

No overrun checking is currently done, because the allocation is in
user memory and anyway the kernel never tries to allocate enough
space in the signal frame yet for an overrun to occur.  This
behaviour will be refined in future patches.

The approach taken in this patch to allocation of the terminator
record is not very clean: this will also be replaced in subsequent
patches.

For future extension, a comment is added in sigcontext.h
documenting the current static allocations in __reserved[].  This
will be important for determining under what circumstances
userspace may or may not see an expanded signal frame.

Signed-off-by: Dave Martin <Dave.Martin@arm.com>
---
 arch/arm64/include/uapi/asm/sigcontext.h | 19 ++++++++++++++
 arch/arm64/kernel/signal.c               | 43 ++++++++++++++++++++++++++------
 2 files changed, 55 insertions(+), 7 deletions(-)

diff --git a/arch/arm64/include/uapi/asm/sigcontext.h b/arch/arm64/include/uapi/asm/sigcontext.h
index ee469be..1328a2c 100644
--- a/arch/arm64/include/uapi/asm/sigcontext.h
+++ b/arch/arm64/include/uapi/asm/sigcontext.h
@@ -34,6 +34,25 @@ struct sigcontext {
 };
 
 /*
+ * Allocation of __reserved[]:
+ * (Note: records do not necessarily occur in the order shown here.)
+ *
+ *	size		description
+ *
+ *	0x210		fpsimd_context
+ *	 0x10		esr_context
+ *	 0x10		terminator (null _aarch64_ctx)
+ *
+ *	0xdd0		(reserved for future allocation)
+ *
+ * New records that can exceed this space need to be opt-in for userspace, so
+ * that an expanded signal frame is not generated unexpectedly.  The mechanism
+ * for opting in will depend on the extension that generates each new record.
+ * The above table documents the maximum set and sizes of records than can be
+ * generated when userspace does not opt in for any such extension.
+ */
+
+/*
  * Header to be used at the beginning of structures extending the user
  * context. Such structures must be placed after the rt_sigframe on the stack
  * and be 16-byte aligned. The last structure must be a dummy one with the
diff --git a/arch/arm64/kernel/signal.c b/arch/arm64/kernel/signal.c
index eaef530..fa787e6 100644
--- a/arch/arm64/kernel/signal.c
+++ b/arch/arm64/kernel/signal.c
@@ -79,6 +79,22 @@ static size_t sigframe_size(struct rt_sigframe_user_layout const *user)
 	return round_up(max(user->size, sizeof(struct rt_sigframe)), 16);
 }
 
+/*
+ * Allocate space for an optional record of <size> bytes in the user
+ * signal frame.  The offset from the signal frame base address to the
+ * allocated block is assigned to *offset.
+ */
+static int sigframe_alloc(struct rt_sigframe_user_layout *user,
+			  unsigned long *offset, size_t size)
+{
+	size_t padded_size = round_up(size, 16);
+
+	*offset = user->size;
+	user->size += padded_size;
+
+	return 0;
+}
+
 static void __user *apply_user_offset(
 	struct rt_sigframe_user_layout const *user, unsigned long offset)
 {
@@ -287,19 +303,32 @@ asmlinkage long sys_rt_sigreturn(struct pt_regs *regs)
 /* Determine the layout of optional records in the signal frame */
 static int setup_sigframe_layout(struct rt_sigframe_user_layout *user)
 {
-	user->fpsimd_offset = user->size;
-	user->size += round_up(sizeof(struct fpsimd_context), 16);
+	int err;
+
+	err = sigframe_alloc(user, &user->fpsimd_offset,
+			     sizeof(struct fpsimd_context));
+	if (err)
+		return err;
 
 	/* fault information, if valid */
 	if (current->thread.fault_code) {
-		user->esr_offset = user->size;
-		user->size += round_up(sizeof(struct esr_context), 16);
+		err = sigframe_alloc(user, &user->esr_offset,
+				     sizeof(struct esr_context));
+		if (err)
+			return err;
 	}
 
-	/* set the "end" magic */
-	user->end_offset = user->size;
+	/*
+	 * Allocate space for the terminator record.
+	 * HACK: here we undo the reservation of space for the end record.
+	 * This bodge should be replaced with a cleaner approach later on.
+	 */
+	user->limit = offsetof(struct rt_sigframe, uc.uc_mcontext.__reserved) +
+		sizeof(user->sigframe->uc.uc_mcontext.__reserved);
 
-	return 0;
+	err = sigframe_alloc(user, &user->end_offset,
+			     sizeof(struct _aarch64_ctx));
+	return err;
 }
 
 
-- 
2.1.4

[PATCH 5/5] arm64: signal: Allow expansion of the signal frame

[Dave Martin]

This patch defines an extra_context signal frame record that can be
used to describe an expanded signal frame, and modifies the context
block allocator and signal frame setup and parsing code to create,
populate, parse and decode this block as necessary.

To avoid abuse by userspace, parse_user_sigframe() attempts to
ensure that:

 * no more than one extra_context is accepted;
 * the extra context data is a sensible size, and properly placed
   and aligned.

The extra_context data is required to start at the first 16-byte
aligned address immediately after the dummy terminator record
following extra_context in rt_sigframe.__reserved[] (as ensured
during signal delivery).  This serves as a sanity-check that the
signal frame has not been moved or copied without taking the extra
data into account.

Signed-off-by: Dave Martin <Dave.Martin@arm.com>
---
 arch/arm64/include/uapi/asm/sigcontext.h |  38 ++++++-
 arch/arm64/kernel/signal.c               | 190 ++++++++++++++++++++++++++++---
 2 files changed, 209 insertions(+), 19 deletions(-)

diff --git a/arch/arm64/include/uapi/asm/sigcontext.h b/arch/arm64/include/uapi/asm/sigcontext.h
index 1328a2c..303a48c 100644
--- a/arch/arm64/include/uapi/asm/sigcontext.h
+++ b/arch/arm64/include/uapi/asm/sigcontext.h
@@ -41,9 +41,10 @@ struct sigcontext {
  *
  *	0x210		fpsimd_context
  *	 0x10		esr_context
+ *	 0x20		extra_context (optional)
  *	 0x10		terminator (null _aarch64_ctx)
  *
- *	0xdd0		(reserved for future allocation)
+ *	0xdb0		(reserved for future allocation)
  *
  * New records that can exceed this space need to be opt-in for userspace, so
  * that an expanded signal frame is not generated unexpectedly.  The mechanism
@@ -80,4 +81,39 @@ struct esr_context {
 	__u64 esr;
 };
 
+/*
+ * extra_context: describes extra space in the signal frame for
+ * additional structures that don't fit in sigcontext.__reserved[].
+ *
+ * Note:
+ *
+ * 1) fpsimd_context, esr_context and extra_context must be placed in
+ * sigcontext.__reserved[] if present.  They cannot be placed in the
+ * extra space.  Any other record can be placed either in the extra
+ * space or in sigcontext.__reserved[], unless otherwise specified in
+ * this file.
+ *
+ * 2) There must not be more than one extra_context.
+ *
+ * 3) If extra_context is present, it must be followed immediately in
+ * sigcontext.__reserved[] by the terminating null _aarch64_ctx.
+ *
+ * 4) The extra space to which data points must start at the first
+ * 16-byte aligned address immediately after the terminating null
+ * _aarch64_ctx that follows the extra_context structure in
+ * __reserved[].  The extra space may overrun the end of __reserved[],
+ * as indicated by a sufficiently large value for the size field.
+ *
+ * 5) The extra space must itself be terminated with a null
+ * _aarch64_ctx.
+ */
+#define EXTRA_MAGIC	0x45585401
+
+struct extra_context {
+	struct _aarch64_ctx head;
+	void __user *data;	/* 16-byte aligned pointer to extra space */
+	__u32 size;		/* size in bytes of the extra space */
+	__u32 __reserved[3];
+};
+
 #endif /* _UAPI__ASM_SIGCONTEXT_H */
diff --git a/arch/arm64/kernel/signal.c b/arch/arm64/kernel/signal.c
index fa787e6..3602fb7 100644
--- a/arch/arm64/kernel/signal.c
+++ b/arch/arm64/kernel/signal.c
@@ -25,6 +25,7 @@
 #include <linux/freezer.h>
 #include <linux/stddef.h>
 #include <linux/uaccess.h>
+#include <linux/sizes.h>
 #include <linux/string.h>
 #include <linux/tracehook.h>
 #include <linux/ratelimit.h>
@@ -60,18 +61,27 @@ struct rt_sigframe_user_layout {
 
 	unsigned long fpsimd_offset;
 	unsigned long esr_offset;
+	unsigned long extra_offset;
 	unsigned long end_offset;
 };
 
+#define BASE_SIGFRAME_SIZE round_up(sizeof(struct rt_sigframe), 16)
+#define TERMINATOR_SIZE round_up(sizeof(struct _aarch64_ctx), 16)
+#define EXTRA_CONTEXT_SIZE round_up(sizeof(struct extra_context), 16)
+
 static void init_user_layout(struct rt_sigframe_user_layout *user)
 {
+	const size_t reserved_size =
+		sizeof(user->sigframe->uc.uc_mcontext.__reserved);
+
 	memset(user, 0, sizeof(*user));
 	user->size = offsetof(struct rt_sigframe, uc.uc_mcontext.__reserved);
 
-	user->limit = user->size +
-		sizeof(user->sigframe->uc.uc_mcontext.__reserved) -
-		round_up(sizeof(struct _aarch64_ctx), 16);
-		/* ^ reserve space for terminator */
+	user->limit = user->size + reserved_size;
+
+	user->limit -= TERMINATOR_SIZE;
+	user->limit -= EXTRA_CONTEXT_SIZE;
+	/* Reserve space for extension and terminator ^ */
 }
 
 static size_t sigframe_size(struct rt_sigframe_user_layout const *user)
@@ -80,6 +90,52 @@ static size_t sigframe_size(struct rt_sigframe_user_layout const *user)
 }
 
 /*
+ * Sanity limit on the approximate maximum size of signal frame we'll
+ * try to generate.  Stack alignment padding and the frame record are
+ * not taken into account.  This limit is not a guarantee and is
+ * NOT ABI.
+ */
+#define SIGFRAME_MAXSZ SZ_64K
+
+static int __sigframe_alloc(struct rt_sigframe_user_layout *user,
+			    unsigned long *offset, size_t size, bool extend)
+{
+	size_t padded_size = round_up(size, 16);
+
+	if (padded_size > user->limit - user->size &&
+	    !user->extra_offset &&
+	    extend) {
+		int ret;
+
+		user->limit += EXTRA_CONTEXT_SIZE;
+		ret = __sigframe_alloc(user, &user->extra_offset,
+				       sizeof(struct extra_context), false);
+		if (ret) {
+			user->limit -= EXTRA_CONTEXT_SIZE;
+			return ret;
+		}
+
+		/* Reserve space for the __reserved[] terminator */
+		user->size += TERMINATOR_SIZE;
+
+		/*
+		 * Allow expansion up to SIGFRAME_MAXSZ, ensuring space for
+		 * the terminator:
+		 */
+		user->limit = SIGFRAME_MAXSZ - TERMINATOR_SIZE;
+	}
+
+	/* Still not enough space?  Bad luck! */
+	if (padded_size > user->limit - user->size)
+		return -ENOMEM;
+
+	*offset = user->size;
+	user->size += padded_size;
+
+	return 0;
+}
+
+/*
  * Allocate space for an optional record of <size> bytes in the user
  * signal frame.  The offset from the signal frame base address to the
  * allocated block is assigned to *offset.
@@ -87,11 +143,24 @@ static size_t sigframe_size(struct rt_sigframe_user_layout const *user)
 static int sigframe_alloc(struct rt_sigframe_user_layout *user,
 			  unsigned long *offset, size_t size)
 {
-	size_t padded_size = round_up(size, 16);
+	return __sigframe_alloc(user, offset, size, true);
+}
 
-	*offset = user->size;
-	user->size += padded_size;
+/* Allocate the null terminator record and prevent further allocations */
+static int sigframe_alloc_end(struct rt_sigframe_user_layout *user)
+{
+	int ret;
+
+	/* Un-reserve the space reserved for the terminator: */
+	user->limit += TERMINATOR_SIZE;
+
+	ret = sigframe_alloc(user, &user->end_offset,
+			     sizeof(struct _aarch64_ctx));
+	if (ret)
+		return ret;
 
+	/* Prevent further allocation: */
+	user->limit = user->size;
 	return 0;
 }
 
@@ -162,6 +231,8 @@ static int parse_user_sigframe(struct user_ctxs *user,
 	char __user *base = (char __user *)&sc->__reserved;
 	size_t offset = 0;
 	size_t limit = sizeof(sc->__reserved);
+	bool have_extra_context = false;
+	char const __user *const sfp = (char const __user *)sf;
 
 	user->fpsimd = NULL;
 
@@ -171,6 +242,12 @@ static int parse_user_sigframe(struct user_ctxs *user,
 	while (1) {
 		int err = 0;
 		u32 magic, size;
+		char const __user *userp;
+		struct extra_context const __user *extra;
+		void __user *extra_data;
+		u32 extra_size;
+		struct _aarch64_ctx const __user *end;
+		u32 end_magic, end_size;
 
 		if (limit - offset < sizeof(*head))
 			goto invalid;
@@ -208,6 +285,64 @@ static int parse_user_sigframe(struct user_ctxs *user,
 			/* ignore */
 			break;
 
+		case EXTRA_MAGIC:
+			if (have_extra_context)
+				goto invalid;
+
+			if (size < sizeof(*extra))
+				goto invalid;
+
+			userp = (char const __user *)head;
+
+			extra = (struct extra_context const __user *)userp;
+			userp += size;
+
+			__get_user_error(extra_data, &extra->data, err);
+			__get_user_error(extra_size, &extra->size, err);
+			if (err)
+				return err;
+
+			/* Check for the dummy terminator in __reserved[]: */
+
+			if (limit - offset - size < TERMINATOR_SIZE)
+				goto invalid;
+
+			end = (struct _aarch64_ctx const __user *)userp;
+			userp += TERMINATOR_SIZE;
+
+			__get_user_error(end_magic, &end->magic, err);
+			__get_user_error(end_size, &end->size, err);
+			if (err)
+				return err;
+
+			if (end_magic || end_size)
+				goto invalid;
+
+			/* Prevent looping/repeated parsing of extra_context */
+			have_extra_context = true;
+
+			base = extra_data;
+			if (!IS_ALIGNED((unsigned long)base, 16))
+				goto invalid;
+
+			if (!IS_ALIGNED(extra_size, 16))
+				goto invalid;
+
+			if (extra_data != userp)
+				goto invalid;
+
+			/* Reject "unreasonably large" frames: */
+			if (extra_size > sfp + SIGFRAME_MAXSZ - userp)
+				goto invalid;
+
+			/*
+			 * Ignore trailing terminator in __reserved[]
+			 * and start parsing extra_data:
+			 */
+			offset = 0;
+			limit = extra_size;
+			continue;
+
 		default:
 			goto invalid;
 		}
@@ -318,17 +453,7 @@ static int setup_sigframe_layout(struct rt_sigframe_user_layout *user)
 			return err;
 	}
 
-	/*
-	 * Allocate space for the terminator record.
-	 * HACK: here we undo the reservation of space for the end record.
-	 * This bodge should be replaced with a cleaner approach later on.
-	 */
-	user->limit = offsetof(struct rt_sigframe, uc.uc_mcontext.__reserved) +
-		sizeof(user->sigframe->uc.uc_mcontext.__reserved);
-
-	err = sigframe_alloc(user, &user->end_offset,
-			     sizeof(struct _aarch64_ctx));
-	return err;
+	return sigframe_alloc_end(user);
 }
 
 
@@ -369,6 +494,35 @@ static int setup_sigframe(struct rt_sigframe_user_layout *user,
 		__put_user_error(current->thread.fault_code, &esr_ctx->esr, err);
 	}
 
+	if (err == 0 && user->extra_offset) {
+		char __user *sfp = (char __user *)user->sigframe;
+		char __user *userp =
+			apply_user_offset(user, user->extra_offset);
+
+		struct extra_context __user *extra;
+		struct _aarch64_ctx __user *end;
+		void __user *extra_data;
+		u32 extra_size;
+
+		extra = (struct extra_context __user *)userp;
+		userp += EXTRA_CONTEXT_SIZE;
+
+		end = (struct _aarch64_ctx __user *)userp;
+		userp += TERMINATOR_SIZE;
+
+		extra_data = userp;
+		extra_size = sfp + round_up(user->size, 16) - userp;
+
+		__put_user_error(EXTRA_MAGIC, &extra->head.magic, err);
+		__put_user_error(EXTRA_CONTEXT_SIZE, &extra->head.size, err);
+		__put_user_error(extra_data, &extra->data, err);
+		__put_user_error(extra_size, &extra->size, err);
+
+		/* Add the terminator */
+		__put_user_error(0, &end->magic, err);
+		__put_user_error(0, &end->size, err);
+	}
+
 	/* set the "end" magic */
 	if (err == 0) {
 		struct _aarch64_ctx __user *end =
-- 
2.1.4

[PATCH 1/5] arm64: signal: split frame link record from sigcontext structure

[Catalin Marinas]

On Thu, Jun 15, 2017 at 03:03:38PM +0100, Dave P Martin wrote:
> In order to be able to increase the amount of the data currently
> written to the __reserved[] array in the signal frame, it is
> necessary to overwrite the locations currently occupied by the
> {fp,lr} frame link record pushed at the top of the signal stack.
> 
> In order for this to work, this patch detaches the frame link
> record from struct rt_sigframe and places it separately at the top
> of the signal stack.  This will allow subsequent patches to insert
> data between it and __reserved[].
> 
> This change relies on the non-ABI status of the placement of the
> frame record with respect to struct sigframe: this status is
> undocumented, but the placement is not declared or described in the
> user headers, and known unwinder implementations (libgcc,
> libunwind, gdb) appear not to rely on it.
> 
> Signed-off-by: Dave Martin <Dave.Martin@arm.com>

I'm fine with this approach:

Reviewed-by: Catalin Marinas <catalin.marinas@arm.com>

[PATCH 2/5] arm64: signal: Refactor sigcontext parsing in rt_sigreturn

[Catalin Marinas]

On Thu, Jun 15, 2017 at 03:03:39PM +0100, Dave P Martin wrote:
> Currently, rt_sigreturn does very limited checking on the
> sigcontext coming from userspace.
> 
> Future additions to the sigcontext data will increase the potential
> for surprises.  Also, it is not clear whether the sigcontext
> extension records are supposed to occur in a particular order.
> 
> To allow the parsing code to be extended more easily, this patch
> factors out the sigcontext parsing into a separate function, and
> adds extra checks to validate the well-formedness of the sigcontext
> structure.
> 
> Signed-off-by: Dave Martin <Dave.Martin@arm.com>

Reviewed-by: Catalin Marinas <catalin.marinas@arm.com>

[PATCH 3/5] arm64: signal: factor frame layout and population into separate passes

[Catalin Marinas]

On Thu, Jun 15, 2017 at 03:03:40PM +0100, Dave P Martin wrote:
> In preparation for expanding the signal frame, this patch refactors
> the signal frame setup code in setup_sigframe() into two separate
> passes.
> 
> The first pass, setup_sigframe_layout(), determines the size of the
> signal frame and its internal layout, including the presence and
> location of optional records.  The resulting knowledge is used to
> allocate and locate the user stack space required for the signal
> frame and to determine which optional records to include.
> 
> The second pass, setup_sigframe(), is called once the stack frame
> is allocated in order to populate it with the necessary context
> information.
> 
> As a result of these changes, it becomes more natural to represent
> locations in the signal frame by a base pointer and an offset,
> since the absolute address of each location is not known during the
> layout pass.  To be more consistent with this logic,
> parse_user_sigframe() is refactored to describe signal frame
> locations in a similar way.
> 
> This change has no effect on the signal ABI, but will make it
> easier to expand the signal frame in future patches.
> 
> Signed-off-by: Dave Martin <Dave.Martin@arm.com>

Reviewed-by: Catalin Marinas <catalin.marinas@arm.com>

[PATCH 4/5] arm64: signal: factor out signal frame record allocation

[Catalin Marinas]

On Thu, Jun 15, 2017 at 03:03:41PM +0100, Dave P Martin wrote:
> This patch factors out the allocator for signal frame optional
> records into a separate function, to ensure consistency and
> facilitate later expansion.
> 
> No overrun checking is currently done, because the allocation is in
> user memory and anyway the kernel never tries to allocate enough
> space in the signal frame yet for an overrun to occur.  This
> behaviour will be refined in future patches.
> 
> The approach taken in this patch to allocation of the terminator
> record is not very clean: this will also be replaced in subsequent
> patches.
> 
> For future extension, a comment is added in sigcontext.h
> documenting the current static allocations in __reserved[].  This
> will be important for determining under what circumstances
> userspace may or may not see an expanded signal frame.
> 
> Signed-off-by: Dave Martin <Dave.Martin@arm.com>

Reviewed-by: Catalin Marinas <catalin.marinas@arm.com>

[PATCH 5/5] arm64: signal: Allow expansion of the signal frame

[Catalin Marinas]

On Thu, Jun 15, 2017 at 03:03:42PM +0100, Dave P Martin wrote:
> This patch defines an extra_context signal frame record that can be
> used to describe an expanded signal frame, and modifies the context
> block allocator and signal frame setup and parsing code to create,
> populate, parse and decode this block as necessary.
> 
> To avoid abuse by userspace, parse_user_sigframe() attempts to
> ensure that:
> 
>  * no more than one extra_context is accepted;
>  * the extra context data is a sensible size, and properly placed
>    and aligned.
> 
> The extra_context data is required to start at the first 16-byte
> aligned address immediately after the dummy terminator record
> following extra_context in rt_sigframe.__reserved[] (as ensured
> during signal delivery).  This serves as a sanity-check that the
> signal frame has not been moved or copied without taking the extra
> data into account.
> 
> Signed-off-by: Dave Martin <Dave.Martin@arm.com>

Reviewed-by: Catalin Marinas <catalin.marinas@arm.com>

[PATCH 1/5] arm64: signal: split frame link record from sigcontext structure

[Dave P Martin]

On Thu, Jun 15, 2017 at 05:37:47PM +0100, Catalin Marinas wrote:
> On Thu, Jun 15, 2017 at 03:03:38PM +0100, Dave P Martin wrote:
> > In order to be able to increase the amount of the data currently
> > written to the __reserved[] array in the signal frame, it is
> > necessary to overwrite the locations currently occupied by the
> > {fp,lr} frame link record pushed at the top of the signal stack.
> >
> > In order for this to work, this patch detaches the frame link
> > record from struct rt_sigframe and places it separately at the top
> > of the signal stack.  This will allow subsequent patches to insert
> > data between it and __reserved[].
> >
> > This change relies on the non-ABI status of the placement of the
> > frame record with respect to struct sigframe: this status is
> > undocumented, but the placement is not declared or described in the
> > user headers, and known unwinder implementations (libgcc,
> > libunwind, gdb) appear not to rely on it.
> >
> > Signed-off-by: Dave Martin <Dave.Martin@arm.com>
>
> I'm fine with this approach:
>
> Reviewed-by: Catalin Marinas <catalin.marinas@arm.com>

Thanks for the review.
Final testing should be complete early next week.

Cheers
---Dave
IMPORTANT NOTICE: The contents of this email and any attachments are confidential and may also be privileged. If you are not the intended recipient, please notify the sender immediately and do not disclose the contents to any other person, use it for any purpose, or store or copy the information in any medium. Thank you.

[PATCH 5/5] arm64: signal: Allow expansion of the signal frame

[Catalin Marinas]

On Thu, Jun 15, 2017 at 03:03:42PM +0100, Dave P Martin wrote:
> --- a/arch/arm64/include/uapi/asm/sigcontext.h
> +++ b/arch/arm64/include/uapi/asm/sigcontext.h
> @@ -41,9 +41,10 @@ struct sigcontext {
>   *
>   *	0x210		fpsimd_context
>   *	 0x10		esr_context
> + *	 0x20		extra_context (optional)
>   *	 0x10		terminator (null _aarch64_ctx)
>   *
> - *	0xdd0		(reserved for future allocation)
> + *	0xdb0		(reserved for future allocation)
>   *
>   * New records that can exceed this space need to be opt-in for userspace, so
>   * that an expanded signal frame is not generated unexpectedly.  The mechanism
> @@ -80,4 +81,39 @@ struct esr_context {
>  	__u64 esr;
>  };
>  
> +/*
> + * extra_context: describes extra space in the signal frame for
> + * additional structures that don't fit in sigcontext.__reserved[].
> + *
> + * Note:
> + *
> + * 1) fpsimd_context, esr_context and extra_context must be placed in
> + * sigcontext.__reserved[] if present.  They cannot be placed in the
> + * extra space.  Any other record can be placed either in the extra
> + * space or in sigcontext.__reserved[], unless otherwise specified in
> + * this file.
> + *
> + * 2) There must not be more than one extra_context.
> + *
> + * 3) If extra_context is present, it must be followed immediately in
> + * sigcontext.__reserved[] by the terminating null _aarch64_ctx.
> + *
> + * 4) The extra space to which data points must start at the first
> + * 16-byte aligned address immediately after the terminating null
> + * _aarch64_ctx that follows the extra_context structure in
> + * __reserved[].  The extra space may overrun the end of __reserved[],
> + * as indicated by a sufficiently large value for the size field.
> + *
> + * 5) The extra space must itself be terminated with a null
> + * _aarch64_ctx.
> + */
> +#define EXTRA_MAGIC	0x45585401
> +
> +struct extra_context {
> +	struct _aarch64_ctx head;
> +	void __user *data;	/* 16-byte aligned pointer to extra space */
> +	__u32 size;		/* size in bytes of the extra space */
> +	__u32 __reserved[3];
> +};

Some more thoughts on the type of "data" above. It's the first time we
introduce pointers in sigcontext and wonder whether __u64 would make
more sense (invariable size with the ABI).

Also for discussion - whether using an offset vs absolute address would
be better.

-- 
Catalin

[PATCH 5/5] arm64: signal: Allow expansion of the signal frame

[Dave Martin]

On Fri, Jun 16, 2017 at 11:47:41AM +0100, Catalin Marinas wrote:
> On Thu, Jun 15, 2017 at 03:03:42PM +0100, Dave P Martin wrote:
> > --- a/arch/arm64/include/uapi/asm/sigcontext.h
> > +++ b/arch/arm64/include/uapi/asm/sigcontext.h
> > @@ -41,9 +41,10 @@ struct sigcontext {
> >   *
> >   *	0x210		fpsimd_context
> >   *	 0x10		esr_context
> > + *	 0x20		extra_context (optional)
> >   *	 0x10		terminator (null _aarch64_ctx)
> >   *
> > - *	0xdd0		(reserved for future allocation)
> > + *	0xdb0		(reserved for future allocation)
> >   *
> >   * New records that can exceed this space need to be opt-in for userspace, so
> >   * that an expanded signal frame is not generated unexpectedly.  The mechanism
> > @@ -80,4 +81,39 @@ struct esr_context {
> >  	__u64 esr;
> >  };
> >  
> > +/*
> > + * extra_context: describes extra space in the signal frame for
> > + * additional structures that don't fit in sigcontext.__reserved[].
> > + *
> > + * Note:
> > + *
> > + * 1) fpsimd_context, esr_context and extra_context must be placed in
> > + * sigcontext.__reserved[] if present.  They cannot be placed in the
> > + * extra space.  Any other record can be placed either in the extra
> > + * space or in sigcontext.__reserved[], unless otherwise specified in
> > + * this file.
> > + *
> > + * 2) There must not be more than one extra_context.
> > + *
> > + * 3) If extra_context is present, it must be followed immediately in
> > + * sigcontext.__reserved[] by the terminating null _aarch64_ctx.
> > + *
> > + * 4) The extra space to which data points must start at the first
> > + * 16-byte aligned address immediately after the terminating null
> > + * _aarch64_ctx that follows the extra_context structure in
> > + * __reserved[].  The extra space may overrun the end of __reserved[],
> > + * as indicated by a sufficiently large value for the size field.
> > + *
> > + * 5) The extra space must itself be terminated with a null
> > + * _aarch64_ctx.
> > + */
> > +#define EXTRA_MAGIC	0x45585401
> > +
> > +struct extra_context {
> > +	struct _aarch64_ctx head;
> > +	void __user *data;	/* 16-byte aligned pointer to extra space */
> > +	__u32 size;		/* size in bytes of the extra space */
> > +	__u32 __reserved[3];
> > +};
> 
> Some more thoughts on the type of "data" above. It's the first time we
> introduce pointers in sigcontext and wonder whether __u64 would make
> more sense (invariable size with the ABI).

I'm happy to change this if you want -- it can be a __u64 without
impacting the semantics.

> Also for discussion - whether using an offset vs absolute address would
> be better.

My rationale here related to the ucontext semantics associated with
the signal frame, rather than signal handling itself.

Consider:

void signal_handler(int n, siginfo_t *si, void *uc)
{
	*signal_context = *(ucontext_t *)uc;
}

Will this "work"?  On most machines, yes it will. 

With an expanded signal frame on arm64, it won't work: the copied
context will be truncated at the end of __reserved[].  If extra_data
uses an offset to locate the additional data then someone parsing
*signal_context won't be able to detect the error and will overrun:
this includes rt_sigreturn (if that is run on a stack that has the
copied context).

If a pointer is used in extra_context, this provides some chance of
handling this: naive code like the above will result in a ucontext_t
whose extra_context data pointer is "wrong": extra_context aware
parsers can detect that and error out or ignore the remaining data.
extra_context aware code that wants to copy the signal context can walk
the signal frame to determine its true size, allocate memory, copy, and
then update the extra_context data pointer in the new copy.

This isn't foolproof, but it at least gives us some protection.

Cheers
---Dave

[PATCH 5/5] arm64: signal: Allow expansion of the signal frame

[Catalin Marinas]

On Mon, Jun 19, 2017 at 11:12:29AM +0100, Dave P Martin wrote:
> On Fri, Jun 16, 2017 at 11:47:41AM +0100, Catalin Marinas wrote:
> > On Thu, Jun 15, 2017 at 03:03:42PM +0100, Dave P Martin wrote:
> > > +struct extra_context {
> > > +	struct _aarch64_ctx head;
> > > +	void __user *data;	/* 16-byte aligned pointer to extra space */
> > > +	__u32 size;		/* size in bytes of the extra space */
> > > +	__u32 __reserved[3];
> > > +};
> > 
> > Some more thoughts on the type of "data" above. It's the first time we
> > introduce pointers in sigcontext and wonder whether __u64 would make
> > more sense (invariable size with the ABI).
> 
> I'm happy to change this if you want -- it can be a __u64 without
> impacting the semantics.

I was thinking in the context of ILP32 but we can address this
separately for the exported kernel headers and glibc definitions.
Looking at this structure from an LP64 perspective, it indeed makes
sense to keep it as void *. We have other UAPI structures with user
pointers already.

> > Also for discussion - whether using an offset vs absolute address would
> > be better.
> 
> My rationale here related to the ucontext semantics associated with
> the signal frame, rather than signal handling itself.
> 
> Consider:
> 
> void signal_handler(int n, siginfo_t *si, void *uc)
> {
> 	*signal_context = *(ucontext_t *)uc;
> }
> 
> Will this "work"?  On most machines, yes it will. 
> 
> With an expanded signal frame on arm64, it won't work: the copied
> context will be truncated at the end of __reserved[].  If extra_data
> uses an offset to locate the additional data then someone parsing
> *signal_context won't be able to detect the error and will overrun:
> this includes rt_sigreturn (if that is run on a stack that has the
> copied context).
> 
> If a pointer is used in extra_context, this provides some chance of
> handling this: naive code like the above will result in a ucontext_t
> whose extra_context data pointer is "wrong": extra_context aware
> parsers can detect that and error out or ignore the remaining data.
> extra_context aware code that wants to copy the signal context can walk
> the signal frame to determine its true size, allocate memory, copy, and
> then update the extra_context data pointer in the new copy.

Thanks for the explanation, it makes sense to keep an absolute pointer
here rather than offset. So no further changes needed to this patch.

-- 
Catalin

[PATCH 5/5] arm64: signal: Allow expansion of the signal frame

[Dave Martin]

On Mon, Jun 19, 2017 at 12:03:47PM +0100, Catalin Marinas wrote:
> On Mon, Jun 19, 2017 at 11:12:29AM +0100, Dave P Martin wrote:
> > On Fri, Jun 16, 2017 at 11:47:41AM +0100, Catalin Marinas wrote:
> > > On Thu, Jun 15, 2017 at 03:03:42PM +0100, Dave P Martin wrote:
> > > > +struct extra_context {
> > > > +	struct _aarch64_ctx head;
> > > > +	void __user *data;	/* 16-byte aligned pointer to extra space */
> > > > +	__u32 size;		/* size in bytes of the extra space */
> > > > +	__u32 __reserved[3];
> > > > +};
> > > 
> > > Some more thoughts on the type of "data" above. It's the first time we
> > > introduce pointers in sigcontext and wonder whether __u64 would make
> > > more sense (invariable size with the ABI).
> > 
> > I'm happy to change this if you want -- it can be a __u64 without
> > impacting the semantics.
> 
> I was thinking in the context of ILP32 but we can address this
> separately for the exported kernel headers and glibc definitions.
> Looking at this structure from an LP64 perspective, it indeed makes
> sense to keep it as void *. We have other UAPI structures with user
> pointers already.

OK -- I'll reply with an additional fixup that can be folded in if you
do want the u64.

> > > Also for discussion - whether using an offset vs absolute address would
> > > be better.
> > 
> > My rationale here related to the ucontext semantics associated with
> > the signal frame, rather than signal handling itself.
> > 
> > Consider:
> > 
> > void signal_handler(int n, siginfo_t *si, void *uc)
> > {
> > 	*signal_context = *(ucontext_t *)uc;
> > }
> > 
> > Will this "work"?  On most machines, yes it will. 
> > 
> > With an expanded signal frame on arm64, it won't work: the copied
> > context will be truncated at the end of __reserved[].  If extra_data
> > uses an offset to locate the additional data then someone parsing
> > *signal_context won't be able to detect the error and will overrun:
> > this includes rt_sigreturn (if that is run on a stack that has the
> > copied context).
> > 
> > If a pointer is used in extra_context, this provides some chance of
> > handling this: naive code like the above will result in a ucontext_t
> > whose extra_context data pointer is "wrong": extra_context aware
> > parsers can detect that and error out or ignore the remaining data.
> > extra_context aware code that wants to copy the signal context can walk
> > the signal frame to determine its true size, allocate memory, copy, and
> > then update the extra_context data pointer in the new copy.
> 
> Thanks for the explanation, it makes sense to keep an absolute pointer
> here rather than offset. So no further changes needed to this patch.

Fair enough.

Cheers
---Dave

[PATCH] fixup! arm64: signal: Allow expansion of the signal frame

[Dave Martin]

Convert extra_context.data to a u64 for better ABI independence.
data is renamed to "datap" to give a stronger clue that it is
a pointer.

The binary interface is unchanged.

Signed-off-by: Dave Martin <Dave.Martin@arm.com>
---

I've build-tested this fixup only.

The only change here is to replace the name "data" with "datap" in
appropriate places and change void __user * to u64 appropriately, with
casts in the two places where this value enters and exits the kernel.

 arch/arm64/include/uapi/asm/sigcontext.h |  6 +++---
 arch/arm64/kernel/signal.c               | 16 ++++++++--------
 2 files changed, 11 insertions(+), 11 deletions(-)

diff --git a/arch/arm64/include/uapi/asm/sigcontext.h b/arch/arm64/include/uapi/asm/sigcontext.h
index 303a48c..f0a76b9 100644
--- a/arch/arm64/include/uapi/asm/sigcontext.h
+++ b/arch/arm64/include/uapi/asm/sigcontext.h
@@ -98,7 +98,7 @@ struct esr_context {
  * 3) If extra_context is present, it must be followed immediately in
  * sigcontext.__reserved[] by the terminating null _aarch64_ctx.
  *
- * 4) The extra space to which data points must start at the first
+ * 4) The extra space to which datap points must start at the first
  * 16-byte aligned address immediately after the terminating null
  * _aarch64_ctx that follows the extra_context structure in
  * __reserved[].  The extra space may overrun the end of __reserved[],
@@ -111,8 +111,8 @@ struct esr_context {
 
 struct extra_context {
 	struct _aarch64_ctx head;
-	void __user *data;	/* 16-byte aligned pointer to extra space */
-	__u32 size;		/* size in bytes of the extra space */
+	__u64 datap; /* 16-byte aligned pointer to extra space cast to __u64 */
+	__u32 size; /* size in bytes of the extra space */
 	__u32 __reserved[3];
 };
 
diff --git a/arch/arm64/kernel/signal.c b/arch/arm64/kernel/signal.c
index 3602fb7..cb0d008 100644
--- a/arch/arm64/kernel/signal.c
+++ b/arch/arm64/kernel/signal.c
@@ -244,7 +244,7 @@ static int parse_user_sigframe(struct user_ctxs *user,
 		u32 magic, size;
 		char const __user *userp;
 		struct extra_context const __user *extra;
-		void __user *extra_data;
+		u64 extra_datap;
 		u32 extra_size;
 		struct _aarch64_ctx const __user *end;
 		u32 end_magic, end_size;
@@ -297,7 +297,7 @@ static int parse_user_sigframe(struct user_ctxs *user,
 			extra = (struct extra_context const __user *)userp;
 			userp += size;
 
-			__get_user_error(extra_data, &extra->data, err);
+			__get_user_error(extra_datap, &extra->datap, err);
 			__get_user_error(extra_size, &extra->size, err);
 			if (err)
 				return err;
@@ -321,14 +321,14 @@ static int parse_user_sigframe(struct user_ctxs *user,
 			/* Prevent looping/repeated parsing of extra_context */
 			have_extra_context = true;
 
-			base = extra_data;
+			base = (void __user *)extra_datap;
 			if (!IS_ALIGNED((unsigned long)base, 16))
 				goto invalid;
 
 			if (!IS_ALIGNED(extra_size, 16))
 				goto invalid;
 
-			if (extra_data != userp)
+			if (base != userp)
 				goto invalid;
 
 			/* Reject "unreasonably large" frames: */
@@ -337,7 +337,7 @@ static int parse_user_sigframe(struct user_ctxs *user,
 
 			/*
 			 * Ignore trailing terminator in __reserved[]
-			 * and start parsing extra_data:
+			 * and start parsing extra data:
 			 */
 			offset = 0;
 			limit = extra_size;
@@ -501,7 +501,7 @@ static int setup_sigframe(struct rt_sigframe_user_layout *user,
 
 		struct extra_context __user *extra;
 		struct _aarch64_ctx __user *end;
-		void __user *extra_data;
+		u64 extra_datap;
 		u32 extra_size;
 
 		extra = (struct extra_context __user *)userp;
@@ -510,12 +510,12 @@ static int setup_sigframe(struct rt_sigframe_user_layout *user,
 		end = (struct _aarch64_ctx __user *)userp;
 		userp += TERMINATOR_SIZE;
 
-		extra_data = userp;
+		extra_datap = (u64)userp;
 		extra_size = sfp + round_up(user->size, 16) - userp;
 
 		__put_user_error(EXTRA_MAGIC, &extra->head.magic, err);
 		__put_user_error(EXTRA_CONTEXT_SIZE, &extra->head.size, err);
-		__put_user_error(extra_data, &extra->data, err);
+		__put_user_error(extra_datap, &extra->datap, err);
 		__put_user_error(extra_size, &extra->size, err);
 
 		/* Add the terminator */
-- 
2.1.4

[PATCH 2/5] arm64: signal: Refactor sigcontext parsing in rt_sigreturn

[Yury Norov]

Hi Dave,

It seems that your series conflicts with one of patches of my ILP32:
https://patchwork.kernel.org/patch/9599015/

Now I try to rebase ilp32 on your patchset, and maybe will have some
questions. The first question is below. If you have comments on my
series - please share it.

Yury

On Thu, Jun 15, 2017 at 03:03:39PM +0100, Dave Martin wrote:
> Currently, rt_sigreturn does very limited checking on the
> sigcontext coming from userspace.
> 
> Future additions to the sigcontext data will increase the potential
> for surprises.  Also, it is not clear whether the sigcontext
> extension records are supposed to occur in a particular order.
> 
> To allow the parsing code to be extended more easily, this patch
> factors out the sigcontext parsing into a separate function, and
> adds extra checks to validate the well-formedness of the sigcontext
> structure.
> 
> Signed-off-by: Dave Martin <Dave.Martin@arm.com>

> +static int parse_user_sigframe(struct user_ctxs *user,
> +			       struct rt_sigframe __user *sf)

You parse sigcontext here in fact. At least you take the pointer to 
the sigcontext from rt_sigframe, and never refer it anymore. rt_sigframe
is different for lp64 and ilp32, but sigcontext is the same. So if you'll 
rename the function to parse_user_sigcontext, and will pass sigcontext
to it directly, I will reuse it in ilp32 code.

> +{
> +	struct sigcontext __user *const sc = &sf->uc.uc_mcontext;
> +	struct _aarch64_ctx __user *head =
> +		(struct _aarch64_ctx __user *)&sc->__reserved;
> +	size_t offset = 0;
> +
> +	user->fpsimd = NULL;
> +
> +	while (1) {
> +		int err;
> +		u32 magic, size;
> +
> +		head = (struct _aarch64_ctx __user *)&sc->__reserved[offset];
> +		if (!IS_ALIGNED((unsigned long)head, 16))
> +			goto invalid;
> +
> +		err = 0;
> +		__get_user_error(magic, &head->magic, err);
> +		__get_user_error(size, &head->size, err);
> +		if (err)
> +			return err;
> +
> +		switch (magic) {
> +		case 0:
> +			if (size)
> +				goto invalid;
> +
> +			goto done;
> +
> +		case FPSIMD_MAGIC:
> +			if (user->fpsimd)
> +				goto invalid;
> +
> +			if (offset > sizeof(sc->__reserved) -
> +					sizeof(*user->fpsimd) ||
> +			    size < sizeof(*user->fpsimd))
> +				goto invalid;
> +
> +			user->fpsimd = (struct fpsimd_context __user *)head;
> +			break;
> +
> +		case ESR_MAGIC:
> +			/* ignore */
> +			break;
> +
> +		default:
> +			goto invalid;
> +		}
> +
> +		if (size < sizeof(*head))
> +			goto invalid;
> +
> +		if (size > sizeof(sc->__reserved) - (sizeof(*head) + offset))
> +			goto invalid;
> +
> +		offset += size;
> +	}
> +
> +done:
> +	if (!user->fpsimd)
> +		goto invalid;
> +
> +	return 0;
> +
> +invalid:
> +	return -EINVAL;
> +}
> +
>  static int restore_sigframe(struct pt_regs *regs,
>  			    struct rt_sigframe __user *sf)
>  {
>  	sigset_t set;
>  	int i, err;
> -	void *aux = sf->uc.uc_mcontext.__reserved;
> +	struct user_ctxs user;
>  
>  	err = __copy_from_user(&set, &sf->uc.uc_sigmask, sizeof(set));
>  	if (err == 0)
> @@ -125,12 +200,11 @@ static int restore_sigframe(struct pt_regs *regs,
>  	regs->syscallno = ~0UL;
>  
>  	err |= !valid_user_regs(&regs->user_regs, current);
> +	if (err == 0)
> +		err = parse_user_sigframe(&user, sf);
>  
> -	if (err == 0) {
> -		struct fpsimd_context *fpsimd_ctx =
> -			container_of(aux, struct fpsimd_context, head);
> -		err |= restore_fpsimd_context(fpsimd_ctx);
> -	}
> +	if (err == 0)
> +		err = restore_fpsimd_context(user.fpsimd);
>  
>  	return err;
>  }
> -- 
> 2.1.4
> 
> 
> _______________________________________________
> linux-arm-kernel mailing list
> linux-arm-kernel at lists.infradead.org
> http://lists.infradead.org/mailman/listinfo/linux-arm-kernel

[PATCH 2/5] arm64: signal: Refactor sigcontext parsing in rt_sigreturn

[Dave Martin]

On Thu, Jun 22, 2017 at 10:32:35PM +0300, Yury Norov wrote:
> Hi Dave,
> 
> It seems that your series conflicts with one of patches of my ILP32:
> https://patchwork.kernel.org/patch/9599015/
> 
> Now I try to rebase ilp32 on your patchset, and maybe will have some
> questions. The first question is below. If you have comments on my
> series - please share it.
> 
> Yury
> 
> On Thu, Jun 15, 2017 at 03:03:39PM +0100, Dave Martin wrote:
> > Currently, rt_sigreturn does very limited checking on the
> > sigcontext coming from userspace.
> > 
> > Future additions to the sigcontext data will increase the potential
> > for surprises.  Also, it is not clear whether the sigcontext
> > extension records are supposed to occur in a particular order.
> > 
> > To allow the parsing code to be extended more easily, this patch
> > factors out the sigcontext parsing into a separate function, and
> > adds extra checks to validate the well-formedness of the sigcontext
> > structure.
> > 
> > Signed-off-by: Dave Martin <Dave.Martin@arm.com>
> 
> > +static int parse_user_sigframe(struct user_ctxs *user,
> > +			       struct rt_sigframe __user *sf)
> 
> You parse sigcontext here in fact. At least you take the pointer to 

That's a fair point...

> the sigcontext from rt_sigframe, and never refer it anymore. rt_sigframe

...however, the final patch [1] (which has not appeared in next yet,
I need to chase it) does refer to the sigframe base in order to
determine when the signal frame exceeds the maximum allowed size.

> is different for lp64 and ilp32, but sigcontext is the same. So if you'll 
> rename the function to parse_user_sigcontext, and will pass sigcontext
> to it directly, I will reuse it in ilp32 code.

Because the rt_sigframe pointer is _only_ used as a base address, the
rt_sigframe definition is not relevant.  So a refactoring along lines of
[2] might work.

Other things that may need attention are the handling of the extra_data
pointer [2] (I changed this from void __user * to u64 after Catalin
pointed out the ABI dependency here), and the way the {fp,lr} frame
record is handled.

I think the RT_SIGFRAME_FP_POS() logic can probably be simplified/
dropped: this is now handled through
rt_sigframe_user_layout.next_frame.  ILP32 would need its own version
of sigframe_size(), or that function should test for current being
ILP32, but the ILP32 equivalent of get_sigframe() would probably look
very similar to the native version after my patches.

(Note that I've not understood the proposed ILP32 signal changes in
detail at this point.)

Cheers
---Dave

[1] [PATCH] arm64: signal: Allow expansion of the signal frame
http://lists.infradead.org/pipermail/linux-arm-kernel/2017-June/514699.html

[2] [RFC PATCH] arm64: signal: Make parse_user_sigframe() independent of rt_sigframe layout
http://lists.infradead.org/pipermail/linux-arm-kernel/2017-June/515361.html

> 
> > +{
> > +	struct sigcontext __user *const sc = &sf->uc.uc_mcontext;
> > +	struct _aarch64_ctx __user *head =
> > +		(struct _aarch64_ctx __user *)&sc->__reserved;
> > +	size_t offset = 0;
> > +
> > +	user->fpsimd = NULL;
> > +
> > +	while (1) {
> > +		int err;
> > +		u32 magic, size;
> > +
> > +		head = (struct _aarch64_ctx __user *)&sc->__reserved[offset];
> > +		if (!IS_ALIGNED((unsigned long)head, 16))
> > +			goto invalid;
> > +
> > +		err = 0;
> > +		__get_user_error(magic, &head->magic, err);
> > +		__get_user_error(size, &head->size, err);
> > +		if (err)
> > +			return err;
> > +
> > +		switch (magic) {
> > +		case 0:
> > +			if (size)
> > +				goto invalid;
> > +
> > +			goto done;
> > +
> > +		case FPSIMD_MAGIC:
> > +			if (user->fpsimd)
> > +				goto invalid;
> > +
> > +			if (offset > sizeof(sc->__reserved) -
> > +					sizeof(*user->fpsimd) ||
> > +			    size < sizeof(*user->fpsimd))
> > +				goto invalid;
> > +
> > +			user->fpsimd = (struct fpsimd_context __user *)head;
> > +			break;
> > +
> > +		case ESR_MAGIC:
> > +			/* ignore */
> > +			break;
> > +
> > +		default:
> > +			goto invalid;
> > +		}
> > +
> > +		if (size < sizeof(*head))
> > +			goto invalid;
> > +
> > +		if (size > sizeof(sc->__reserved) - (sizeof(*head) + offset))
> > +			goto invalid;
> > +
> > +		offset += size;
> > +	}
> > +
> > +done:
> > +	if (!user->fpsimd)
> > +		goto invalid;
> > +
> > +	return 0;
> > +
> > +invalid:
> > +	return -EINVAL;
> > +}
> > +
> >  static int restore_sigframe(struct pt_regs *regs,
> >  			    struct rt_sigframe __user *sf)
> >  {
> >  	sigset_t set;
> >  	int i, err;
> > -	void *aux = sf->uc.uc_mcontext.__reserved;
> > +	struct user_ctxs user;
> >  
> >  	err = __copy_from_user(&set, &sf->uc.uc_sigmask, sizeof(set));
> >  	if (err == 0)
> > @@ -125,12 +200,11 @@ static int restore_sigframe(struct pt_regs *regs,
> >  	regs->syscallno = ~0UL;
> >  
> >  	err |= !valid_user_regs(&regs->user_regs, current);
> > +	if (err == 0)
> > +		err = parse_user_sigframe(&user, sf);
> >  
> > -	if (err == 0) {
> > -		struct fpsimd_context *fpsimd_ctx =
> > -			container_of(aux, struct fpsimd_context, head);
> > -		err |= restore_fpsimd_context(fpsimd_ctx);
> > -	}
> > +	if (err == 0)
> > +		err = restore_fpsimd_context(user.fpsimd);
> >  
> >  	return err;
> >  }
> > -- 
> > 2.1.4
> > 
> > 
> > _______________________________________________
> > linux-arm-kernel mailing list
> > linux-arm-kernel at lists.infradead.org
> > http://lists.infradead.org/mailman/listinfo/linux-arm-kernel
> 
> _______________________________________________
> linux-arm-kernel mailing list
> linux-arm-kernel at lists.infradead.org
> http://lists.infradead.org/mailman/listinfo/linux-arm-kernel