On Mon, Jun 05, 2017 at 03:17:44PM +0100, Catalin Marinas wrote: > 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 Yes, except that when the signal frame is generated by the kernel those will be the same, since the kernel generates a frame that is only as large as needed for the records dumped. I'm thinking of the case where userspace, or a debugger, modifies a frame obtained from a signal, or similar. This might involve deleting or resizing records. Encoding the original size of the frame may help code defend against overrun bugs, though obviously it's no guarantee. > 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. Sure, it can be a u64. I wanted to avoid the suggestion that the frame should be that large, but 32 bits already allows it to be crazy large anyway, so I don't think making it 32-bit helps. > > > 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. If the size field is retained I prefer to keep this, but it's deliberately not in any header. This allows the kernel to have a stricter idea about what is sane, without it formally being ABI. This is supposed to be a deterrent against people writing signal frame code manipulation code in a stupid way. SIGFRAME_MAXSZ should only ever be increased during maintenance -- it's probably worth adding a comment on that point. Cheers ---Dave
