| Message ID | 1492016495-19689-4-git-send-email-Dave.Martin@arm.com (mailing list archive) |
|---|---|
| State | New, archived |
| Headers | show |
Hi Dave, On Wed, Apr 12, 2017 at 06:01:13PM +0100, Dave P Martin wrote: > --- a/arch/arm64/include/uapi/asm/sigcontext.h > +++ b/arch/arm64/include/uapi/asm/sigcontext.h > @@ -80,4 +80,31 @@ struct esr_context { > __u64 esr; > }; > > +/* > + * Pointer to extra space for additional structures that don't fit in > + * sigcontext.__reserved[]. Note: > + * > + * 1) fpsimd_context, esr_context and extra_context must be placed in > + * sigcontext.__reserved[] if present. They cannot be placed in the > + * extra space. Any other record can be placed either in the extra > + * space or in sigcontext.__reserved[]. > + * > + * 2) There must not be more than one extra_context. > + * > + * 3) If extra_context is present, it must be followed immediately in > + * sigcontext.__reserved[] by the terminating null _aarch64_ctx (i.e., > + * extra_context must be the last record in sigcontext.__reserved[] > + * except for the terminator). > + * > + * 4) The extra space must itself be terminated with a null > + * _aarch64_ctx. > + */ IIUC, if we need to save some state that doesn't fit in what's left of sigcontext.__reserved[] (e.g. SVE with 1024-bit vector length), we ignore the available space and go for a memory block following the end of sigcontext.__reserved[] + 16. Is there a reason we can't store the new state across the end of sigcontext.__reserved[] and move fp/lr at the end of the new frame? I'm not sure the fp/lr position immediately after __reserved[] counts as ABI. > +#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. > + __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.
On Fri, May 12, 2017 at 05:57:24PM +0100, Catalin Marinas wrote: > Hi Dave, > > On Wed, Apr 12, 2017 at 06:01:13PM +0100, Dave P Martin wrote: > > --- a/arch/arm64/include/uapi/asm/sigcontext.h > > +++ b/arch/arm64/include/uapi/asm/sigcontext.h > > @@ -80,4 +80,31 @@ struct esr_context { > > __u64 esr; > > }; > > > > +/* > > + * Pointer to extra space for additional structures that don't fit in > > + * sigcontext.__reserved[]. Note: > > + * > > + * 1) fpsimd_context, esr_context and extra_context must be placed in > > + * sigcontext.__reserved[] if present. They cannot be placed in the > > + * extra space. Any other record can be placed either in the extra > > + * space or in sigcontext.__reserved[]. > > + * > > + * 2) There must not be more than one extra_context. > > + * > > + * 3) If extra_context is present, it must be followed immediately in > > + * sigcontext.__reserved[] by the terminating null _aarch64_ctx (i.e., > > + * extra_context must be the last record in sigcontext.__reserved[] > > + * except for the terminator). > > + * > > + * 4) The extra space must itself be terminated with a null > > + * _aarch64_ctx. > > + */ > > IIUC, if we need to save some state that doesn't fit in what's left of > sigcontext.__reserved[] (e.g. SVE with 1024-bit vector length), we > ignore the available space and go for a memory block following the end > of sigcontext.__reserved[] + 16. Is there a reason we can't store the > new state across the end of sigcontext.__reserved[] and move fp/lr at > the end of the new frame? I'm not sure the fp/lr position immediately > after __reserved[] counts as ABI. This was my original view. 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. We could also move fp/lr later, if we require sigframe parsers to be a bit more permissive about where extra_context starts. > > +#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__. data really is a pointer to a user address. Compare for example struct iovec in include/uapi/linux/uio.h. I think the __user is required to keep sparse happy, otherwise some additional casting is needed to bless a valid read from this field before passing it to get/put_user. Or there may be some other reason I've forgotten... > > > + __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. The size parameter *may* provide some limited defence against sigreturn-mediated stack smashing in userspace, but it wasn't designed for this purpose and again the value is debatable. We can likely live without it if you're not keen on it. Cheers ---Dave
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: > > > --- a/arch/arm64/include/uapi/asm/sigcontext.h > > > +++ b/arch/arm64/include/uapi/asm/sigcontext.h > > > @@ -80,4 +80,31 @@ struct esr_context { > > > __u64 esr; > > > }; > > > > > > +/* > > > + * Pointer to extra space for additional structures that don't fit in > > > + * sigcontext.__reserved[]. Note: > > > + * > > > + * 1) fpsimd_context, esr_context and extra_context must be placed in > > > + * sigcontext.__reserved[] if present. They cannot be placed in the > > > + * extra space. Any other record can be placed either in the extra > > > + * space or in sigcontext.__reserved[]. > > > + * > > > + * 2) There must not be more than one extra_context. > > > + * > > > + * 3) If extra_context is present, it must be followed immediately in > > > + * sigcontext.__reserved[] by the terminating null _aarch64_ctx (i.e., > > > + * extra_context must be the last record in sigcontext.__reserved[] > > > + * except for the terminator). > > > + * > > > + * 4) The extra space must itself be terminated with a null > > > + * _aarch64_ctx. > > > + */ > > > > IIUC, if we need to save some state that doesn't fit in what's left of > > sigcontext.__reserved[] (e.g. SVE with 1024-bit vector length), we > > ignore the available space and go for a memory block following the end > > of sigcontext.__reserved[] + 16. Is there a reason we can't store the > > new state across the end of sigcontext.__reserved[] and move fp/lr at > > the end of the new frame? I'm not sure the fp/lr position immediately > > after __reserved[] counts as ABI. > > This was my original view. > > 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. > > > +#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. > > > + __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. 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?
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
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.
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
On Tue, Jun 06, 2017 at 12:37:53PM +0100, Dave Martin wrote: > On Mon, Jun 05, 2017 at 03:17:44PM +0100, Catalin Marinas wrote: [...] [extra_context.size] > > 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. Actually, there is still implicit padding even with u64, since the total size is 16 bytes + sizeof(extra_context.size). Since u64 is much bigger then we'd ever want, and to avoid introducing new bugs, do you object to keeping size as u32 and adding explicit padding instead? Cheers ---Dave
On Tue, Jun 06, 2017 at 02:58:18PM +0100, Dave P Martin wrote: > On Tue, Jun 06, 2017 at 12:37:53PM +0100, Dave Martin wrote: > > On Mon, Jun 05, 2017 at 03:17:44PM +0100, Catalin Marinas wrote: > > [...] > > [extra_context.size] > > > > 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. > > Actually, there is still implicit padding even with u64, since the total > size is 16 bytes + sizeof(extra_context.size). > > Since u64 is much bigger then we'd ever want, and to avoid introducing > new bugs, do you object to keeping size as u32 and adding explicit > padding instead? I missed the extra padding to 16-byte alignment. In this case, I'm fine with u32.
On Tue, Jun 06, 2017 at 12:37:53PM +0100, Dave P Martin wrote: > 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: > > > > 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. Fine by me.
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: > > > > --- a/arch/arm64/include/uapi/asm/sigcontext.h > > > > +++ b/arch/arm64/include/uapi/asm/sigcontext.h > > > > @@ -80,4 +80,31 @@ struct esr_context { > > > > __u64 esr; > > > > }; > > > > > > > > +/* > > > > + * Pointer to extra space for additional structures that don't fit in > > > > + * sigcontext.__reserved[]. Note: > > > > + * > > > > + * 1) fpsimd_context, esr_context and extra_context must be placed in > > > > + * sigcontext.__reserved[] if present. They cannot be placed in the > > > > + * extra space. Any other record can be placed either in the extra > > > > + * space or in sigcontext.__reserved[]. > > > > + * > > > > + * 2) There must not be more than one extra_context. > > > > + * > > > > + * 3) If extra_context is present, it must be followed immediately in > > > > + * sigcontext.__reserved[] by the terminating null _aarch64_ctx (i.e., > > > > + * extra_context must be the last record in sigcontext.__reserved[] > > > > + * except for the terminator). > > > > + * > > > > + * 4) The extra space must itself be terminated with a null > > > > + * _aarch64_ctx. > > > > + */ > > > > > > IIUC, if we need to save some state that doesn't fit in what's left of > > > sigcontext.__reserved[] (e.g. SVE with 1024-bit vector length), we > > > ignore the available space and go for a memory block following the end > > > of sigcontext.__reserved[] + 16. Is there a reason we can't store the > > > new state across the end of sigcontext.__reserved[] and move fp/lr at > > > the end of the new frame? I'm not sure the fp/lr position immediately > > > after __reserved[] counts as ABI. > > > > This was my original view. > > > > 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. On this point, it looks like the libgcc unwinder https://gcc.gnu.org/git/?p=gcc.git;a=blob;f=libgcc/config/aarch64/linux-unwind.h;h=d5d6980442fd47b1f1e499e99cb25b5fffbdbeb3;hb=HEAD doesn't rely on the frame record location. It clones the (internal) struct rt_sigframe definition from v3.7, which doesn't include any frame record, and mines fp and lr out of the signal frame AFAICT. It appears that gdb and libunwind likely take the same approach, but I've not looked closely yet. The frame record in rt_sigframe was added in by Will in 304ef4e83672 ("arm64: signal: push the unwinding prologue on the signal stack"), which changes from pushing the frame record onto the interrupted stack (which may be inaccessible for a SEGV), to pushing onto the signal stack. Even with the frame record split from rt_sigframe, I've not seen any failed backtrace in gdb. Throwing an exception from a SEGV handler in C++ (with -fnon-call-exceptions) also appears to work reliably with that change, even when the signal frame grows. In any case, there is no ABI break unless there is extra_context, so it shouldn't impact current userspace. Cheers ---Dave
diff --git a/arch/arm64/include/uapi/asm/sigcontext.h b/arch/arm64/include/uapi/asm/sigcontext.h index 1328a2c..b5e2523 100644 --- a/arch/arm64/include/uapi/asm/sigcontext.h +++ b/arch/arm64/include/uapi/asm/sigcontext.h @@ -80,4 +80,31 @@ struct esr_context { __u64 esr; }; +/* + * Pointer to extra space for additional structures that don't fit in + * sigcontext.__reserved[]. Note: + * + * 1) fpsimd_context, esr_context and extra_context must be placed in + * sigcontext.__reserved[] if present. They cannot be placed in the + * extra space. Any other record can be placed either in the extra + * space or in sigcontext.__reserved[]. + * + * 2) There must not be more than one extra_context. + * + * 3) If extra_context is present, it must be followed immediately in + * sigcontext.__reserved[] by the terminating null _aarch64_ctx (i.e., + * extra_context must be the last record in sigcontext.__reserved[] + * except for the terminator). + * + * 4) The extra space must itself be terminated with a null + * _aarch64_ctx. + */ +#define EXTRA_MAGIC 0x45585401 + +struct extra_context { + struct _aarch64_ctx head; + void __user *data; /* 16-byte aligned pointer to extra space */ + __u32 size; /* size in bytes of the extra space */ +}; + #endif /* _UAPI__ASM_SIGCONTEXT_H */ diff --git a/arch/arm64/kernel/signal.c b/arch/arm64/kernel/signal.c index 47592af..95547e1 100644 --- a/arch/arm64/kernel/signal.c +++ b/arch/arm64/kernel/signal.c @@ -25,6 +25,7 @@ #include <linux/freezer.h> #include <linux/stddef.h> #include <linux/uaccess.h> +#include <linux/sizes.h> #include <linux/string.h> #include <linux/tracehook.h> #include <linux/ratelimit.h> @@ -56,18 +57,27 @@ struct rt_sigframe_user_layout { unsigned long fpsimd_offset; unsigned long esr_offset; + unsigned long extra_offset; unsigned long end_offset; }; +#define BASE_SIGFRAME_SIZE round_up(sizeof(struct rt_sigframe), 16) +#define TERMINATOR_SIZE round_up(sizeof(struct _aarch64_ctx), 16) +#define EXTRA_CONTEXT_SIZE round_up(sizeof(struct extra_context), 16) + static void init_user_layout(struct rt_sigframe_user_layout *user) { + const size_t reserved_size = + sizeof(user->sigframe->uc.uc_mcontext.__reserved); + memset(user, 0, sizeof(*user)); user->size = offsetof(struct rt_sigframe, uc.uc_mcontext.__reserved); - user->limit = user->size + - sizeof(user->sigframe->uc.uc_mcontext.__reserved) - - round_up(sizeof(struct _aarch64_ctx), 16); - /* ^ reserve space for terminator */ + user->limit = user->size + reserved_size; + + user->limit -= TERMINATOR_SIZE; + user->limit -= EXTRA_CONTEXT_SIZE; + /* Reserve space for extension and terminator ^ */ } static size_t sigframe_size(struct rt_sigframe_user_layout const *user) @@ -75,6 +85,48 @@ static size_t sigframe_size(struct rt_sigframe_user_layout const *user) return round_up(max(user->size, sizeof(struct rt_sigframe)), 16); } +/* Sanity limit on the maximum size of signal frame we'll try to generate. */ +/* This is NOT ABI. */ +#define SIGFRAME_MAXSZ SZ_64K + +static int __sigframe_alloc(struct rt_sigframe_user_layout *user, + unsigned long *offset, size_t size, bool extend) +{ + size_t padded_size = round_up(size, 16); + + if (padded_size > user->limit - user->size && + !user->extra_offset && + extend) { + int ret; + + ret = __sigframe_alloc(user, &user->extra_offset, + sizeof(struct extra_context), false); + if (ret) + return ret; + + /* + * Further allocations must go after the fixed-size + * part of the signal frame: + */ + user->size = BASE_SIGFRAME_SIZE; + + /* + * Allow expansion up to SIGFRAME_MAXSZ, ensuring space for + * the terminator: + */ + user->limit = SIGFRAME_MAXSZ - TERMINATOR_SIZE; + } + + /* Still not enough space? Bad luck! */ + if (padded_size > user->limit - user->size) + return -ENOMEM; + + *offset = user->size; + user->size += padded_size; + + return 0; +} + /* * Allocate space for an optional record of <size> bytes in the user * signal frame. The offset from the signal frame base address to the @@ -83,11 +135,24 @@ static size_t sigframe_size(struct rt_sigframe_user_layout const *user) static int sigframe_alloc(struct rt_sigframe_user_layout *user, unsigned long *offset, size_t size) { - size_t padded_size = round_up(size, 16); + return __sigframe_alloc(user, offset, size, true); +} - *offset = user->size; - user->size += padded_size; +/* Allocate the null terminator record and prevent further allocations */ +static int sigframe_alloc_end(struct rt_sigframe_user_layout *user) +{ + int ret; + + /* Un-reserve the space reserved for the terminator: */ + user->limit += TERMINATOR_SIZE; + + ret = sigframe_alloc(user, &user->end_offset, + sizeof(struct _aarch64_ctx)); + if (ret) + return ret; + /* Prevent further allocation: */ + user->limit = user->size; return 0; } @@ -314,17 +379,7 @@ static int setup_sigframe_layout(struct rt_sigframe_user_layout *user) return err; } - /* - * Allocate space for the terminator record. - * HACK: here we undo the reservation of space for the end record. - * This bodge should be replaced with a cleaner approach later on. - */ - user->limit = offsetof(struct rt_sigframe, uc.uc_mcontext.__reserved) + - sizeof(user->sigframe->uc.uc_mcontext.__reserved); - - err = sigframe_alloc(user, &user->end_offset, - sizeof(struct _aarch64_ctx)); - return err; + return sigframe_alloc_end(user); } @@ -365,6 +420,26 @@ static int setup_sigframe(struct rt_sigframe_user_layout *user, __put_user_error(current->thread.fault_code, &esr_ctx->esr, err); } + if (err == 0 && user->extra_offset) { + struct extra_context __user *extra = + apply_user_offset(user, user->extra_offset); + struct _aarch64_ctx __user *end = + (struct _aarch64_ctx __user *)((char __user *)extra + + round_up(sizeof(*extra), 16)); + void __user *extra_data = + apply_user_offset(user, BASE_SIGFRAME_SIZE); + u32 extra_size = round_up(user->size, 16) - BASE_SIGFRAME_SIZE; + + __put_user_error(EXTRA_MAGIC, &extra->head.magic, err); + __put_user_error(sizeof(*extra), &extra->head.size, err); + __put_user_error(extra_data, &extra->data, err); + __put_user_error(extra_size, &extra->size, err); + + /* Add the terminator */ + __put_user_error(0, &end->magic, err); + __put_user_error(0, &end->size, err); + } + /* set the "end" magic */ if (err == 0) { struct _aarch64_ctx __user *end =
This patch modifies the context block allocator to create an extra_context expansion block as necessary, and adds the necessary code to populate, parse and decode this block. Signed-off-by: Dave Martin <Dave.Martin@arm.com> --- arch/arm64/include/uapi/asm/sigcontext.h | 27 ++++++++ arch/arm64/kernel/signal.c | 111 ++++++++++++++++++++++++++----- 2 files changed, 120 insertions(+), 18 deletions(-)