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
