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:

quoted

On Mon, May 15, 2017 at 02:24:45PM +0100, Dave P Martin wrote:

quoted

On Fri, May 12, 2017 at 05:57:24PM +0100, Catalin Marinas wrote:

quoted

On Wed, Apr 12, 2017 at 06:01:13PM +0100, Dave P Martin wrote:

quoted

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

quoted

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