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:

quoted

On Fri, Jun 16, 2017 at 11:47:41AM +0100, Catalin Marinas wrote:

quoted

On Thu, Jun 15, 2017 at 03:03:42PM +0100, Dave P Martin wrote:

quoted

+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.

quoted

quoted

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