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

[Dave Martin <Dave.Martin@arm.com>]

On Tue, May 23, 2017 at 12:30:19PM +0100, Catalin Marinas wrote:
> On Mon, May 15, 2017 at 02:24:45PM +0100, Dave P Martin wrote:
> > On Fri, May 12, 2017 at 05:57:24PM +0100, Catalin Marinas wrote:
> > > On Wed, Apr 12, 2017 at 06:01:13PM +0100, Dave P Martin wrote:

[...]

> > 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.
> 
> I would not consider this ABI. The ABI part is that the fp register
> points to where fp/lr were saved.

Fair enough, I'm happy to move it back.  (That was my preferred design
in the first place anyway).

> > > > +#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__.
> 
> Ah, ok, I missed this.

It was a surprise to me too...

> > > > +	__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.
> 
> I'm not keen on the size information, it seems superfluous.

Are you referring to the fact that fp will point to the end of
extra_context, or simply that we don't really need to know the size?


The first is only true of the signal frame.  For a sigcontext on the
heap (e.g., getcontext/setcontext) the frame record has no role and
nothing would point to it, so it's no use for locating the end of
extra_context.  In practice there won't be a frame record in this
situation.

The second is a valid point, just as we don't need to know the size of
the destination buffer of strcpy() or sprintf().  The programmer can
ensure that it's big enough.  Moreover, from the kernel we're also
protected by uaccess.

It's easy to screw up here though.  setcontext assumes that mcontext_t
is fixed-size, so if there is no embedded size information then there
is no way to protect against overruns.  In userspace, there is no
uaccess protection and we can simply walk across the end of a heap
block etc.

> BTW, does SIGFRAME_MAXSZ now become ABI? Or the user only needs to
> interrogate the frame size and we keep this internal to the kernel?

If the kernel rejects extra_contexts that cause this limit to be
exceeded, then yes -- though it will rarely be relevant except in the
case of memory corruption, or if architecture extensions eventually
require a larger frame.

(sve_context could theoretically grow larger then SIGFRAME_MAXSZ all by
itself, but that's unlikely to happen any time soon.)


Userspace could hit SIGFRAME_MAXSZ by constructing a valid sequence of
records that is ridiculously large, by padding out the records: common
sense suggests not to do this, but it's never been documented or
enforced.  I didn't feel comfortable changing the behaviour here to be
more strict.


So, SIGFRAME_MAXSZ should either be given a larger, more future-proof
value ... or otherwise we should perhaps get rid of it entirely.

?

Cheers
---Dave