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

[Catalin Marinas]

On Fri, May 26, 2017 at 12:37:32PM +0100, Dave P Martin wrote:
> 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:
> > > > > +	__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 latter, I don't think we need to know the size explicitly, we can
add up the chained structures to calculate it.

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

So you intend size to be the maximum size, not the actual size of the
chained contexts? If that's the case, I think we can keep it, only that
I'd rather have the time size_t or __u64 to avoid implicit padding.

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

If we can, yes, I would get rid of it.

-- 
Catalin