[RFC PATCH v2 4/6] arm64: signal: Allocate extra sigcontext space as needed

[Dave Martin]

On Fri, May 12, 2017 at 05:57:24PM +0100, Catalin Marinas wrote:
> Hi Dave,
> 
> On Wed, Apr 12, 2017 at 06:01:13PM +0100, Dave P Martin wrote:
> > --- a/arch/arm64/include/uapi/asm/sigcontext.h
> > +++ b/arch/arm64/include/uapi/asm/sigcontext.h
> > @@ -80,4 +80,31 @@ struct esr_context {
> >  	__u64 esr;
> >  };
> >  
> > +/*
> > + * Pointer to extra space 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[].
> > + *
> > + * 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 (i.e.,
> > + * extra_context must be the last record in sigcontext.__reserved[]
> > + * except for the terminator).
> > + *
> > + * 4) The extra space must itself be terminated with a null
> > + * _aarch64_ctx.
> > + */
> 
> IIUC, if we need to save some state that doesn't fit in what's left of
> sigcontext.__reserved[] (e.g. SVE with 1024-bit vector length), we
> ignore the available space and go for a memory block following the end
> of sigcontext.__reserved[] + 16. Is there a reason we can't store the
> new state across the end of sigcontext.__reserved[] and move fp/lr at
> the end of the new frame? I'm not sure the fp/lr position immediately
> after __reserved[] counts as ABI.

This was my original view.

Originally I preferred not to waste the space and did move fp/lr to the
end, but someone (I think you or Will) expressed concern that the fp/lr
position relative to the signal frame _might_ count as ABI.

I think it's not that likely that software will be relying on this,
since it appears easier just to follow the frame chain than to treat
this as a special case.

But it's hard to be certain.  It comes down to a judgement call.

We could also move fp/lr later, if we require sigframe parsers to be
a bit more permissive about where extra_context starts.

> > +#define EXTRA_MAGIC	0x45585401
> > +
> > +struct extra_context {
> > +	struct _aarch64_ctx head;
> > +	void __user *data;	/* 16-byte aligned pointer to extra space */
> "__user" is a kernel-only attribute, we shouldn't expose it in a uapi
> header.

This is filtered out by headers_install, just like #ifdef __KERNEL__.

data really is a pointer to a user address.  Compare for example struct
iovec in include/uapi/linux/uio.h.

I think the __user is required to keep sparse happy, otherwise some
additional casting is needed to bless a valid read from this field
before passing it to get/put_user.  Or there may be some other reason
I've forgotten...

> 
> > +	__u32 size;		/* size in bytes of the extra space */
> > +};
> 
> Do we need the size of the extra space? Can we not infer it anyway by
> walking the contexts save there? Surely we don't expect more than one
> extra context.

Strictly speaking we don't need it.  When userspace parses a signal
frame generated by the kernel, it can trust the kernel to write a well-
formed signal frame.

In sigreturn it allows us to retain a sanity-check on overall size
similar to what sizeof(__reserved) gives us.  This "feels cleaner"
to me, but the value of it is debatable, since we can still apply
SIGFRAME_MAXSZ and uaccess should protect us against gross overruns.

The size parameter *may* provide some limited defence against
sigreturn-mediated stack smashing in userspace, but it wasn't designed
for this purpose and again the value is debatable.

We can likely live without it if you're not keen on it.

Cheers
---Dave