Re: [PATCH v7 5/6] x86/signal: Detect and prevent an alternate signal stack overflow
On Wed, Apr 14, 2021 at 01:30:43PM +0200, Florian Weimer wrote: > Is this discussion about better behavior (at least diagnostics) for > existing applications, without any code changes? Or an alternative > programming model? Former. > Does noavx512 acutally reduce the XSAVE size to AVX2 levels? Yeah. > Or would you need noxsave? I don't think so. > One possibility is that the sigaltstack size check prevents application > from running which work just fine today because all they do is install a > stack overflow handler, and stack overflow does not actually happen. So sigaltstack(2) says in the NOTES: Functions called from a signal handler executing on an alternate signal stack will also use the alternate signal stack. (This also applies to any handlers invoked for other signals while the process is executing on the alternate signal stack.) Unlike the standard stack, the system does not automatically extend the alternate signal stack. Exceeding the allocated size of the alternate signal stack will lead to unpredictable results. > So if sigaltstack fails and the application checks the result of the > system call, it probably won't run at all. Shifting the diagnostic to > the pointer where the signal would have to be delivered is perhaps the > only thing that can be done. So using the example from the same manpage: The most common usage of an alternate signal stack is to handle the SIGSEGV sig‐ nal that is generated if the space available for the normal process stack is ex‐ hausted: in this case, a signal handler for SIGSEGV cannot be invoked on the process stack; if we wish to handle it, we must use an alternate signal stack. and considering these "unpredictable results" would it make sense or even be at all possible to return SIGFAIL from that SIGSEGV signal handler which should run on the sigaltstack but that sigaltstack overflows? I think we wanna be able to tell the process through that previously registered SIGSEGV handler which is supposed to run on the sigaltstack, that that stack got overflowed. Or is this use case obsolete and this is not what people do at all? > As for SIGFAIL in particular, I don't think there are any leftover > signal numbers. It would need a prctl to assign the signal number, and > I'm not sure if there is a useful programming model because signals do > not really compose well even today. SIGFAIL adds another point where > libraries need to collaborate, and we do not have a mechanism for that. > (This is about what Rich Felker termed “library-safe code”, proper > maintenance of process-wide resources such as the current directory.) Oh fun. I guess if Linux goes and does something, people would adopt it and it'll become standard. :-P Thx. -- Regards/Gruss, Boris. https://people.kernel.org/tglx/notes-about-netiquette
Re: [PATCH v7 5/6] x86/signal: Detect and prevent an alternate signal stack overflow
* Borislav Petkov: > On Mon, Apr 12, 2021 at 10:30:23PM +, Bae, Chang Seok wrote: >> On Mar 26, 2021, at 03:30, Borislav Petkov wrote: >> > On Thu, Mar 25, 2021 at 09:56:53PM -0700, Andy Lutomirski wrote: >> >> We really ought to have a SIGSIGFAIL signal that's sent, double-fault >> >> style, when we fail to send a signal. >> > >> > Yeap, we should be able to tell userspace that we couldn't send a >> > signal, hohumm. >> >> Hi Boris, >> >> Let me clarify some details as preparing to include this in a revision. >> >> So, IIUC, a number needs to be assigned for this new SIGFAIL. At a glance, >> not >> sure which one to pick there in signal.h -- 1-31 fully occupied and the rest >> for 33 different real-time signals. >> >> Also, perhaps, force_sig(SIGFAIL) here, instead of return -1 -- to die with >> SIGSEGV. > > I think this needs to be decided together with userspace people so that > they can act accordingly and whether it even makes sense to them. > > Florian, any suggestions? Is this discussion about better behavior (at least diagnostics) for existing applications, without any code changes? Or an alternative programming model? Does noavx512 acutally reduce the XSAVE size to AVX2 levels? Or would you need noxsave? One possibility is that the sigaltstack size check prevents application from running which work just fine today because all they do is install a stack overflow handler, and stack overflow does not actually happen. So if sigaltstack fails and the application checks the result of the system call, it probably won't run at all. Shifting the diagnostic to the pointer where the signal would have to be delivered is perhaps the only thing that can be done. As for SIGFAIL in particular, I don't think there are any leftover signal numbers. It would need a prctl to assign the signal number, and I'm not sure if there is a useful programming model because signals do not really compose well even today. SIGFAIL adds another point where libraries need to collaborate, and we do not have a mechanism for that. (This is about what Rich Felker termed “library-safe code”, proper maintenance of process-wide resources such as the current directory.) Thanks, Florian
Re: [PATCH v7 5/6] x86/signal: Detect and prevent an alternate signal stack overflow
On Mon, Apr 12, 2021 at 10:30:23PM +, Bae, Chang Seok wrote: > On Mar 26, 2021, at 03:30, Borislav Petkov wrote: > > On Thu, Mar 25, 2021 at 09:56:53PM -0700, Andy Lutomirski wrote: > >> We really ought to have a SIGSIGFAIL signal that's sent, double-fault > >> style, when we fail to send a signal. > > > > Yeap, we should be able to tell userspace that we couldn't send a > > signal, hohumm. > > Hi Boris, > > Let me clarify some details as preparing to include this in a revision. > > So, IIUC, a number needs to be assigned for this new SIGFAIL. At a glance, not > sure which one to pick there in signal.h -- 1-31 fully occupied and the rest > for 33 different real-time signals. > > Also, perhaps, force_sig(SIGFAIL) here, instead of return -1 -- to die with > SIGSEGV. I think this needs to be decided together with userspace people so that they can act accordingly and whether it even makes sense to them. Florian, any suggestions? Subthread starts here: https://lkml.kernel.org/r/calcetrxqzuvjqrhdmst6ppgtjxas_spk2jhwmimdnpunq45...@mail.gmail.com Thx. -- Regards/Gruss, Boris. https://people.kernel.org/tglx/notes-about-netiquette
Re: [PATCH v7 5/6] x86/signal: Detect and prevent an alternate signal stack overflow
On Mar 26, 2021, at 03:30, Borislav Petkov wrote: > On Thu, Mar 25, 2021 at 09:56:53PM -0700, Andy Lutomirski wrote: >> We really ought to have a SIGSIGFAIL signal that's sent, double-fault >> style, when we fail to send a signal. > > Yeap, we should be able to tell userspace that we couldn't send a > signal, hohumm. Hi Boris, Let me clarify some details as preparing to include this in a revision. So, IIUC, a number needs to be assigned for this new SIGFAIL. At a glance, not sure which one to pick there in signal.h -- 1-31 fully occupied and the rest for 33 different real-time signals. Also, perhaps, force_sig(SIGFAIL) here, instead of return -1 -- to die with SIGSEGV. Thanks, Chang
Re: [PATCH v7 5/6] x86/signal: Detect and prevent an alternate signal stack overflow
On Thu, Mar 25, 2021 at 09:56:53PM -0700, Andy Lutomirski wrote: > Nope. on_sig_stack() is a horrible kludge and won't work here. We > could have something like __on_sig_stack() or sp_is_on_sig_stack() or > something, though. Yeah, see my other reply. Ack to either of those carved out helpers. > I figure that the people whose programs spontaneously crash should get > a hint why if they look at dmesg. Maybe the message should say > "overflowed sigaltstack -- try noavx512"? I guess, as long as it is ratelimited. I mean, we can remove it later if it starts gettin' annoying. > We really ought to have a SIGSIGFAIL signal that's sent, double-fault > style, when we fail to send a signal. Yeap, we should be able to tell userspace that we couldn't send a signal, hohumm. Thx. -- Regards/Gruss, Boris. https://people.kernel.org/tglx/notes-about-netiquette
Re: [PATCH v7 5/6] x86/signal: Detect and prevent an alternate signal stack overflow
I forgot to mention why I cc'd all you fine Xen folk: On Thu, Mar 25, 2021 at 11:13 AM Andy Lutomirski wrote: > > > } else if (IS_ENABLED(CONFIG_X86_32) && > >!onsigstack && > >regs->ss != __USER_DS && This bit here seems really dubious on Xen PV. Honestly it seems dubious everywhere, but especially on Xen PV. --Andy
Re: [PATCH v7 5/6] x86/signal: Detect and prevent an alternate signal stack overflow
On Thu, Mar 25, 2021 at 11:54 AM Borislav Petkov wrote:
>
> On Thu, Mar 25, 2021 at 11:13:12AM -0700, Andy Lutomirski wrote:
> > diff --git a/arch/x86/kernel/signal.c b/arch/x86/kernel/signal.c
> > index ea794a083c44..53781324a2d3 100644
> > --- a/arch/x86/kernel/signal.c
> > +++ b/arch/x86/kernel/signal.c
> > @@ -237,7 +237,8 @@ get_sigframe(struct k_sigaction *ka, struct pt_regs
> > *regs, size_t frame_size,
> > unsigned long math_size = 0;
> > unsigned long sp = regs->sp;
> > unsigned long buf_fx = 0;
> > - int onsigstack = on_sig_stack(sp);
> > + bool already_onsigstack = on_sig_stack(sp);
> > + bool entering_altstack = false;
> > int ret;
> >
> > /* redzone */
> > @@ -246,15 +247,25 @@ get_sigframe(struct k_sigaction *ka, struct pt_regs
> > *regs, size_t frame_size,
> >
> > /* This is the X/Open sanctioned signal stack switching. */
> > if (ka->sa.sa_flags & SA_ONSTACK) {
> > - if (sas_ss_flags(sp) == 0)
> > + /*
> > + * This checks already_onsigstack via sas_ss_flags().
> > + * Sensible programs use SS_AUTODISARM, which disables
> > + * that check, and programs that don't use
> > + * SS_AUTODISARM get compatible but potentially
> > + * bizarre behavior.
> > + */
> > + if (sas_ss_flags(sp) == 0) {
> > sp = current->sas_ss_sp + current->sas_ss_size;
> > + entering_altstack = true;
> > + }
> > } else if (IS_ENABLED(CONFIG_X86_32) &&
> > -!onsigstack &&
> > +!already_onsigstack &&
> > regs->ss != __USER_DS &&
> > !(ka->sa.sa_flags & SA_RESTORER) &&
> > ka->sa.sa_restorer) {
> > /* This is the legacy signal stack switching. */
> > sp = (unsigned long) ka->sa.sa_restorer;
> > + entering_altstack = true;
> > }
>
> What a mess this whole signal handling is. I need a course in signal
> handling to understand what's going on here...
>
> >
> > sp = fpu__alloc_mathframe(sp, IS_ENABLED(CONFIG_X86_32),
> > @@ -267,8 +278,16 @@ get_sigframe(struct k_sigaction *ka, struct pt_regs
> > *regs, size_t frame_size,
> >* If we are on the alternate signal stack and would overflow it,
> > don't.
> >* Return an always-bogus address instead so we will die with SIGSEGV.
> >*/
> > - if (onsigstack && !likely(on_sig_stack(sp)))
> > + if (unlikely(entering_altstack &&
> > + (sp <= current->sas_ss_sp ||
> > + sp - current->sas_ss_sp > current->sas_ss_size))) {
>
> You could've simply done
>
> if (unlikely(entering_altstack && !on_sig_stack(sp)))
>
> here.
Nope. on_sig_stack() is a horrible kludge and won't work here. We
could have something like __on_sig_stack() or sp_is_on_sig_stack() or
something, though.
>
>
> > + if (show_unhandled_signals && printk_ratelimit()) {
> > + pr_info("%s[%d] overflowed sigaltstack",
> > + tsk->comm, task_pid_nr(tsk));
> > + }
>
> Why do you even wanna issue that? It looks like callers will propagate
> an error value up and people don't look at dmesg all the time.
I figure that the people whose programs spontaneously crash should get
a hint why if they look at dmesg. Maybe the message should say
"overflowed sigaltstack -- try noavx512"?
We really ought to have a SIGSIGFAIL signal that's sent, double-fault
style, when we fail to send a signal.
Re: [PATCH v7 5/6] x86/signal: Detect and prevent an alternate signal stack overflow
On Thu, Mar 25, 2021 at 09:11:56PM +, Bae, Chang Seok wrote: > But if sigaltstack()’ed with the SS_AUTODISARM flag, both on_sig_stack() and > sas_ss_flags() return 0 [1]. Then, segfault always here. v5 had the exact > issue before [2]. Ah, there's that SS_AUTODISARM check above it which I missed, sorry. I guess we can do a __on_sig_stack() helper or so which does the stack check only without the SS_AUTODISARM. Just for readability's sake in what is already a pretty messy function. Thx. -- Regards/Gruss, Boris. SUSE Software Solutions Germany GmbH, GF: Felix Imendörffer, HRB 36809, AG Nürnberg
Re: [PATCH v7 5/6] x86/signal: Detect and prevent an alternate signal stack overflow
On Mar 25, 2021, at 11:54, Borislav Petkov wrote:
> On Thu, Mar 25, 2021 at 11:13:12AM -0700, Andy Lutomirski wrote:
>> diff --git a/arch/x86/kernel/signal.c b/arch/x86/kernel/signal.c
>> index ea794a083c44..53781324a2d3 100644
>> --- a/arch/x86/kernel/signal.c
>> +++ b/arch/x86/kernel/signal.c
>> @@ -237,7 +237,8 @@ get_sigframe(struct k_sigaction *ka, struct pt_regs
>> *regs, size_t frame_size,
>> unsigned long math_size = 0;
>> unsigned long sp = regs->sp;
>> unsigned long buf_fx = 0;
>> -int onsigstack = on_sig_stack(sp);
>> +bool already_onsigstack = on_sig_stack(sp);
>> +bool entering_altstack = false;
>> int ret;
>>
>> /* redzone */
>> @@ -246,15 +247,25 @@ get_sigframe(struct k_sigaction *ka, struct pt_regs
>> *regs, size_t frame_size,
>>
>> /* This is the X/Open sanctioned signal stack switching. */
>> if (ka->sa.sa_flags & SA_ONSTACK) {
>> -if (sas_ss_flags(sp) == 0)
>> +/*
>> + * This checks already_onsigstack via sas_ss_flags().
>> + * Sensible programs use SS_AUTODISARM, which disables
>> + * that check, and programs that don't use
>> + * SS_AUTODISARM get compatible but potentially
>> + * bizarre behavior.
>> + */
>> +if (sas_ss_flags(sp) == 0) {
>> sp = current->sas_ss_sp + current->sas_ss_size;
>> +entering_altstack = true;
>> +}
>> } else if (IS_ENABLED(CONFIG_X86_32) &&
>> - !onsigstack &&
>> + !already_onsigstack &&
>> regs->ss != __USER_DS &&
>> !(ka->sa.sa_flags & SA_RESTORER) &&
>> ka->sa.sa_restorer) {
>> /* This is the legacy signal stack switching. */
>> sp = (unsigned long) ka->sa.sa_restorer;
>> +entering_altstack = true;
>> }
>
> What a mess this whole signal handling is. I need a course in signal
> handling to understand what's going on here...
>
>>
>> sp = fpu__alloc_mathframe(sp, IS_ENABLED(CONFIG_X86_32),
>> @@ -267,8 +278,16 @@ get_sigframe(struct k_sigaction *ka, struct pt_regs
>> *regs, size_t frame_size,
>> * If we are on the alternate signal stack and would overflow it, don't.
>> * Return an always-bogus address instead so we will die with SIGSEGV.
>> */
>> -if (onsigstack && !likely(on_sig_stack(sp)))
>> +if (unlikely(entering_altstack &&
>> + (sp <= current->sas_ss_sp ||
>> + sp - current->sas_ss_sp > current->sas_ss_size))) {
>
> You could've simply done
>
> if (unlikely(entering_altstack && !on_sig_stack(sp)))
>
> here.
But if sigaltstack()’ed with the SS_AUTODISARM flag, both on_sig_stack() and
sas_ss_flags() return 0 [1]. Then, segfault always here. v5 had the exact
issue before [2].
[1]
https://git.kernel.org/pub/scm/linux/kernel/git/torvalds/linux.git/tree/include/linux/sched/signal.h#n576
[2]
https://lore.kernel.org/lkml/CALCETrXuFrHUU-L=HMofTgEDZk9muPnVtK=ejsthqq01xhb...@mail.gmail.com/
Thanks,
Chang
Re: [PATCH v7 5/6] x86/signal: Detect and prevent an alternate signal stack overflow
* Chang Seok via Libc-alpha Bae:
> On Mar 25, 2021, at 09:20, Borislav Petkov wrote:
>>
>> $ gcc tst-minsigstksz-2.c -DMY_MINSIGSTKSZ=3453 -o tst-minsigstksz-2
>> $ ./tst-minsigstksz-2
>> tst-minsigstksz-2: changed byte 50 bytes below configured stack
>>
>> Whoops.
>>
>> And the debug print said:
>>
>> [ 5395.252884] signal: get_sigframe: sp: 0x7f54ec39e7b8, sas_ss_sp:
>> 0x7f54ec39e6ce, sas_ss_size 0xd7d
>>
>> which tells me that, AFAICT, your check whether we have enough alt stack
>> doesn't seem to work in this case.
>
> Yes, in this case.
>
> tst-minsigstksz-2.c has this code:
>
> static void
> handler (int signo)
> {
> /* Clear a bit of on-stack memory. */
> volatile char buffer[256];
> for (size_t i = 0; i < sizeof (buffer); ++i)
> buffer[i] = 0;
> handler_run = 1;
> }
> …
>
> if (handler_run != 1)
> errx (1, "handler did not run");
>
> for (void *p = stack_buffer; p < stack_bottom; ++p)
> if (*(unsigned char *) p != 0xCC)
> errx (1, "changed byte %zd bytes below configured stack\n",
> stack_bottom - p);
> …
>
> I think the message comes from the handler’s overwriting, not from the kernel.
>
> The patch's check is to detect and prevent the kernel-induced overflow --
> whether alt stack enough for signal delivery itself. The stack is possibly
> not enough for the signal handler's use as the kernel does not know for it.
Ahh, right. When I wrote the test, I didn't know which turn the
kernel would eventually take, so the test is quite arbitrary.
The glibc dynamic loader uses XSAVE/XSAVEC as well, so you can
probably double the practical stack requirement if lazy binding is in
use and can be triggered from the signal handler. Estimating stack
sizes is hard.
Re: [PATCH v7 5/6] x86/signal: Detect and prevent an alternate signal stack overflow
On Thu, Mar 25, 2021 at 11:13:12AM -0700, Andy Lutomirski wrote:
> diff --git a/arch/x86/kernel/signal.c b/arch/x86/kernel/signal.c
> index ea794a083c44..53781324a2d3 100644
> --- a/arch/x86/kernel/signal.c
> +++ b/arch/x86/kernel/signal.c
> @@ -237,7 +237,8 @@ get_sigframe(struct k_sigaction *ka, struct pt_regs
> *regs, size_t frame_size,
> unsigned long math_size = 0;
> unsigned long sp = regs->sp;
> unsigned long buf_fx = 0;
> - int onsigstack = on_sig_stack(sp);
> + bool already_onsigstack = on_sig_stack(sp);
> + bool entering_altstack = false;
> int ret;
>
> /* redzone */
> @@ -246,15 +247,25 @@ get_sigframe(struct k_sigaction *ka, struct pt_regs
> *regs, size_t frame_size,
>
> /* This is the X/Open sanctioned signal stack switching. */
> if (ka->sa.sa_flags & SA_ONSTACK) {
> - if (sas_ss_flags(sp) == 0)
> + /*
> + * This checks already_onsigstack via sas_ss_flags().
> + * Sensible programs use SS_AUTODISARM, which disables
> + * that check, and programs that don't use
> + * SS_AUTODISARM get compatible but potentially
> + * bizarre behavior.
> + */
> + if (sas_ss_flags(sp) == 0) {
> sp = current->sas_ss_sp + current->sas_ss_size;
> + entering_altstack = true;
> + }
> } else if (IS_ENABLED(CONFIG_X86_32) &&
> -!onsigstack &&
> +!already_onsigstack &&
> regs->ss != __USER_DS &&
> !(ka->sa.sa_flags & SA_RESTORER) &&
> ka->sa.sa_restorer) {
> /* This is the legacy signal stack switching. */
> sp = (unsigned long) ka->sa.sa_restorer;
> + entering_altstack = true;
> }
What a mess this whole signal handling is. I need a course in signal
handling to understand what's going on here...
>
> sp = fpu__alloc_mathframe(sp, IS_ENABLED(CONFIG_X86_32),
> @@ -267,8 +278,16 @@ get_sigframe(struct k_sigaction *ka, struct pt_regs
> *regs, size_t frame_size,
>* If we are on the alternate signal stack and would overflow it, don't.
>* Return an always-bogus address instead so we will die with SIGSEGV.
>*/
> - if (onsigstack && !likely(on_sig_stack(sp)))
> + if (unlikely(entering_altstack &&
> + (sp <= current->sas_ss_sp ||
> + sp - current->sas_ss_sp > current->sas_ss_size))) {
You could've simply done
if (unlikely(entering_altstack && !on_sig_stack(sp)))
here.
> + if (show_unhandled_signals && printk_ratelimit()) {
> + pr_info("%s[%d] overflowed sigaltstack",
> + tsk->comm, task_pid_nr(tsk));
> + }
Why do you even wanna issue that? It looks like callers will propagate
an error value up and people don't look at dmesg all the time.
Btw, s/tsk/current/g
IOW, this builds:
---
diff --git a/arch/x86/kernel/signal.c b/arch/x86/kernel/signal.c
index a06cb107c0e8..c00e932b5f18 100644
--- a/arch/x86/kernel/signal.c
+++ b/arch/x86/kernel/signal.c
@@ -234,10 +234,11 @@ get_sigframe(struct k_sigaction *ka, struct pt_regs
*regs, size_t frame_size,
void __user **fpstate)
{
/* Default to using normal stack */
+ bool already_onsigstack = on_sig_stack(regs->sp);
+ bool entering_altstack = false;
unsigned long math_size = 0;
unsigned long sp = regs->sp;
unsigned long buf_fx = 0;
- int onsigstack = on_sig_stack(sp);
int ret;
/* redzone */
@@ -246,15 +247,24 @@ get_sigframe(struct k_sigaction *ka, struct pt_regs
*regs, size_t frame_size,
/* This is the X/Open sanctioned signal stack switching. */
if (ka->sa.sa_flags & SA_ONSTACK) {
- if (sas_ss_flags(sp) == 0)
+ /*
+* This checks already_onsigstack via sas_ss_flags(). Sensible
+* programs use SS_AUTODISARM, which disables that check, and
+* programs that don't use SS_AUTODISARM get compatible but
+* potentially bizarre behavior.
+*/
+ if (sas_ss_flags(sp) == 0) {
sp = current->sas_ss_sp + current->sas_ss_size;
+ entering_altstack = true;
+ }
} else if (IS_ENABLED(CONFIG_X86_32) &&
- !onsigstack &&
+ !already_onsigstack &&
regs->ss != __USER_DS &&
!(ka->sa.sa_flags & SA_RESTORER) &&
ka->sa.sa_restorer) {
/* This is the legacy signal stack switching. */
sp = (unsigned long) ka->sa.sa_restorer;
+ entering_altstack = true;
}
sp = fpu__alloc_mathframe(sp, IS_ENABLED(CONFIG_X86_32),
@@ -267,8 +277,14 @@ get_sigframe(struct k_sigaction
Re: [PATCH v7 5/6] x86/signal: Detect and prevent an alternate signal stack overflow
On Mon, Mar 15, 2021 at 11:57 PM Chang S. Bae wrote:
>
> The kernel pushes context on to the userspace stack to prepare for the
> user's signal handler. When the user has supplied an alternate signal
> stack, via sigaltstack(2), it is easy for the kernel to verify that the
> stack size is sufficient for the current hardware context.
>
> Check if writing the hardware context to the alternate stack will exceed
> it's size. If yes, then instead of corrupting user-data and proceeding with
> the original signal handler, an immediate SIGSEGV signal is delivered.
>
> Instead of calling on_sig_stack(), directly check the new stack pointer
> whether in the bounds.
>
> While the kernel allows new source code to discover and use a sufficient
> alternate signal stack size, this check is still necessary to protect
> binaries with insufficient alternate signal stack size from data
> corruption.
This patch results in excessively complicated control and data flow.
> - int onsigstack = on_sig_stack(sp);
> + bool onsigstack = on_sig_stack(sp);
Here onsigstack means "we were already using the altstack".
> int ret;
>
> /* redzone */
> @@ -251,8 +251,11 @@ get_sigframe(struct k_sigaction *ka, struct pt_regs
> *regs, size_t frame_size,
>
> /* This is the X/Open sanctioned signal stack switching. */
> if (ka->sa.sa_flags & SA_ONSTACK) {
> - if (sas_ss_flags(sp) == 0)
> + if (sas_ss_flags(sp) == 0) {
> sp = current->sas_ss_sp + current->sas_ss_size;
> + /* On the alternate signal stack */
> + onsigstack = true;
> + }
But now onsigstack is also true if we are using the legacy path to
*enter* the altstack. So now it's (was on altstack) || (entering
altstack via legacy path).
> } else if (IS_ENABLED(CONFIG_X86_32) &&
>!onsigstack &&
>regs->ss != __USER_DS &&
> @@ -272,7 +275,8 @@ get_sigframe(struct k_sigaction *ka, struct pt_regs
> *regs, size_t frame_size,
> * If we are on the alternate signal stack and would overflow it,
> don't.
> * Return an always-bogus address instead so we will die with SIGSEGV.
> */
> - if (onsigstack && !likely(on_sig_stack(sp)))
> + if (onsigstack && unlikely(sp <= current->sas_ss_sp ||
> + sp - current->sas_ss_sp >
> current->sas_ss_size))
And now we fail if ((was on altstack) || (entering altstack via legacy
path)) && (new sp is out of bounds).
The condition we actually want is that, if we are entering the
altstack and we don't fit, we should fail. This is tricky because of
the autodisarm stuff and the possibility of nonlinear stack segments,
so it's not even clear to me exactly what we should be doing. I
propose:
> return (void __user *)-1L;
Can we please log something (if (show_unhandled_signals ||
printk_ratelimit()) that says that we overflowed the altstack?
How about:
diff --git a/arch/x86/kernel/signal.c b/arch/x86/kernel/signal.c
index ea794a083c44..53781324a2d3 100644
--- a/arch/x86/kernel/signal.c
+++ b/arch/x86/kernel/signal.c
@@ -237,7 +237,8 @@ get_sigframe(struct k_sigaction *ka, struct
pt_regs *regs, size_t frame_size,
unsigned long math_size = 0;
unsigned long sp = regs->sp;
unsigned long buf_fx = 0;
-int onsigstack = on_sig_stack(sp);
+bool already_onsigstack = on_sig_stack(sp);
+bool entering_altstack = false;
int ret;
/* redzone */
@@ -246,15 +247,25 @@ get_sigframe(struct k_sigaction *ka, struct
pt_regs *regs, size_t frame_size,
/* This is the X/Open sanctioned signal stack switching. */
if (ka->sa.sa_flags & SA_ONSTACK) {
-if (sas_ss_flags(sp) == 0)
+/*
+ * This checks already_onsigstack via sas_ss_flags().
+ * Sensible programs use SS_AUTODISARM, which disables
+ * that check, and programs that don't use
+ * SS_AUTODISARM get compatible but potentially
+ * bizarre behavior.
+ */
+if (sas_ss_flags(sp) == 0) {
sp = current->sas_ss_sp + current->sas_ss_size;
+entering_altstack = true;
+}
} else if (IS_ENABLED(CONFIG_X86_32) &&
- !onsigstack &&
+ !already_onsigstack &&
regs->ss != __USER_DS &&
!(ka->sa.sa_flags & SA_RESTORER) &&
ka->sa.sa_restorer) {
/* This is the legacy signal stack switching. */
sp = (unsigned long) ka->sa.sa_restorer;
+entering_altstack = true;
}
sp = fpu__alloc_mathframe(sp, IS_ENABLED(CONFIG_X86_32),
@@ -267,8 +278,16 @@ get_sigframe(struct k_sigaction *ka, struct
pt_regs *regs, size_t frame_size,
* If we are on the alternate signal stack and would overflow it, don't.
* Return an always-bogus address instead so we will die with SIGSEGV.
*/
-if (onsigstack && !likely(on_sig_stack(sp)))
Re: [PATCH v7 5/6] x86/signal: Detect and prevent an alternate signal stack overflow
On Mar 25, 2021, at 09:20, Borislav Petkov wrote:
>
> $ gcc tst-minsigstksz-2.c -DMY_MINSIGSTKSZ=3453 -o tst-minsigstksz-2
> $ ./tst-minsigstksz-2
> tst-minsigstksz-2: changed byte 50 bytes below configured stack
>
> Whoops.
>
> And the debug print said:
>
> [ 5395.252884] signal: get_sigframe: sp: 0x7f54ec39e7b8, sas_ss_sp:
> 0x7f54ec39e6ce, sas_ss_size 0xd7d
>
> which tells me that, AFAICT, your check whether we have enough alt stack
> doesn't seem to work in this case.
Yes, in this case.
tst-minsigstksz-2.c has this code:
static void
handler (int signo)
{
/* Clear a bit of on-stack memory. */
volatile char buffer[256];
for (size_t i = 0; i < sizeof (buffer); ++i)
buffer[i] = 0;
handler_run = 1;
}
…
if (handler_run != 1)
errx (1, "handler did not run");
for (void *p = stack_buffer; p < stack_bottom; ++p)
if (*(unsigned char *) p != 0xCC)
errx (1, "changed byte %zd bytes below configured stack\n",
stack_bottom - p);
…
I think the message comes from the handler’s overwriting, not from the kernel.
The patch's check is to detect and prevent the kernel-induced overflow --
whether alt stack enough for signal delivery itself. The stack is possibly
not enough for the signal handler's use as the kernel does not know for it.
Thanks,
Chang
Re: [PATCH v7 5/6] x86/signal: Detect and prevent an alternate signal stack overflow
On Tue, Mar 16, 2021 at 06:26:46PM +, Bae, Chang Seok wrote:
> I suspect the AVX-512 states not enabled there.
Ok, I found a machine which has AVX-512:
[0.00] x86/fpu: Supporting XSAVE feature 0x001: 'x87 floating point
registers'
[0.00] x86/fpu: Supporting XSAVE feature 0x002: 'SSE registers'
[0.00] x86/fpu: Supporting XSAVE feature 0x004: 'AVX registers'
[0.00] x86/fpu: Supporting XSAVE feature 0x020: 'AVX-512 opmask'
[0.00] x86/fpu: Supporting XSAVE feature 0x040: 'AVX-512 Hi256'
[0.00] x86/fpu: Supporting XSAVE feature 0x080: 'AVX-512 ZMM_Hi256'
[0.00] x86/fpu: Supporting XSAVE feature 0x200: 'Protection Keys User
registers'
[0.00] x86/fpu: xstate_offset[2]: 576, xstate_sizes[2]: 256
[0.00] x86/fpu: xstate_offset[5]: 832, xstate_sizes[5]: 64
[0.00] x86/fpu: xstate_offset[6]: 896, xstate_sizes[6]: 512
[0.00] x86/fpu: xstate_offset[7]: 1408, xstate_sizes[7]: 1024
[0.00] x86/fpu: xstate_offset[9]: 2432, xstate_sizes[9]:8
[0.00] x86/fpu: Enabled xstate features 0x2e7, context size is 2440
bytes, using 'compacted' format.
and applied your patch and added a debug printk, see end of mail.
Then, I ran the test case:
$ gcc tst-minsigstksz-2.c -DMY_MINSIGSTKSZ=3453 -o tst-minsigstksz-2
$ ./tst-minsigstksz-2
tst-minsigstksz-2: changed byte 50 bytes below configured stack
Whoops.
And the debug print said:
[ 5395.252884] signal: get_sigframe: sp: 0x7f54ec39e7b8, sas_ss_sp:
0x7f54ec39e6ce, sas_ss_size 0xd7d
which tells me that, AFAICT, your check whether we have enough alt stack
doesn't seem to work in this case.
Thx.
diff --git a/arch/x86/kernel/signal.c b/arch/x86/kernel/signal.c
index a06cb107c0e8..a7396f7c3832 100644
--- a/arch/x86/kernel/signal.c
+++ b/arch/x86/kernel/signal.c
@@ -237,7 +237,7 @@ get_sigframe(struct k_sigaction *ka, struct pt_regs *regs,
size_t frame_size,
unsigned long math_size = 0;
unsigned long sp = regs->sp;
unsigned long buf_fx = 0;
- int onsigstack = on_sig_stack(sp);
+ bool onsigstack = on_sig_stack(sp);
int ret;
/* redzone */
@@ -246,8 +246,11 @@ get_sigframe(struct k_sigaction *ka, struct pt_regs *regs,
size_t frame_size,
/* This is the X/Open sanctioned signal stack switching. */
if (ka->sa.sa_flags & SA_ONSTACK) {
- if (sas_ss_flags(sp) == 0)
+ if (sas_ss_flags(sp) == 0) {
sp = current->sas_ss_sp + current->sas_ss_size;
+ /* On the alternate signal stack */
+ onsigstack = true;
+ }
} else if (IS_ENABLED(CONFIG_X86_32) &&
!onsigstack &&
regs->ss != __USER_DS &&
@@ -263,11 +266,16 @@ get_sigframe(struct k_sigaction *ka, struct pt_regs
*regs, size_t frame_size,
sp = align_sigframe(sp - frame_size);
+ if (onsigstack)
+ pr_info("%s: sp: 0x%lx, sas_ss_sp: 0x%lx, sas_ss_size 0x%lx\n",
+ __func__, sp, current->sas_ss_sp, current->sas_ss_size);
+
/*
* If we are on the alternate signal stack and would overflow it, don't.
* Return an always-bogus address instead so we will die with SIGSEGV.
*/
- if (onsigstack && !likely(on_sig_stack(sp)))
+ if (onsigstack && unlikely(sp <= current->sas_ss_sp ||
+ sp - current->sas_ss_sp >
current->sas_ss_size))
return (void __user *)-1L;
/* save i387 and extended state */
--
Regards/Gruss,
Boris.
SUSE Software Solutions Germany GmbH, GF: Felix Imendörffer, HRB 36809, AG
Nürnberg
Re: [PATCH v7 5/6] x86/signal: Detect and prevent an alternate signal stack overflow
On Mar 16, 2021, at 04:52, Borislav Petkov wrote: > On Mon, Mar 15, 2021 at 11:52:14PM -0700, Chang S. Bae wrote: >> @@ -272,7 +275,8 @@ get_sigframe(struct k_sigaction *ka, struct pt_regs >> *regs, size_t frame_size, >> * If we are on the alternate signal stack and would overflow it, don't. >> * Return an always-bogus address instead so we will die with SIGSEGV. >> */ >> -if (onsigstack && !likely(on_sig_stack(sp))) >> +if (onsigstack && unlikely(sp <= current->sas_ss_sp || >> + sp - current->sas_ss_sp > >> current->sas_ss_size)) >> return (void __user *)-1L; > > So clearly I'm missing something because trying to trigger the test case > in the bugzilla: > > https://bugzilla.kernel.org/show_bug.cgi?id=153531 > > on current tip/master doesn't work. Runs with MY_MINSIGSTKSZ under 2048 > fail with: > > tst-minsigstksz-2: sigaltstack: Cannot allocate memory > > and above 2048 don't overwrite bytes below the stack. > > So something else is missing. How did you test this patch? I suspect the AVX-512 states not enabled there. When I ran it under a machine without AVX-512 like this, it didn’t show the overwrite message: $ cat /proc/cpuinfo | grep -m1 "model name” model name : Intel(R) Core(TM) i9-10900K CPU @ 3.70GHz $ sudo dmesg | grep "Enabled xstate” [0.00] x86/fpu: Enabled xstate features 0x1f, context size is 960 bytes, using ‘compacted’ format. $ gcc tst-minsigstksz-2.c -DMY_MINSIGSTKSZ=2047 $ ./a.out a.out: sigaltstack: Cannot allocate memory $ gcc tst-minsigstksz-2.c -DMY_MINSIGSTKSZ=2048 $ ./a.out When do it again with AVX-512, it did show the message: $ cat /proc/cpuinfo | grep -m1 "model name” model name : Intel(R) Core(TM) i9-7940X CPU @ 3.10GHz $ sudo dmesg | grep "Enabled xstate” [0.00] x86/fpu: Enabled xstate features 0xff, context size is 2560 bytes, using 'compacted' format. $ gcc tst-minsigstksz-2.c -DMY_MINSIGSTKSZ=2048 $ ./a.out a.out: changed byte 1412 bytes below configured stack $ gcc tst-minsigstksz-2.c -DMY_MINSIGSTKSZ=3490 $ ./a.out a.out: changed byte 21 bytes below configured stack $ gcc tst-minsigstksz-2.c -DMY_MINSIGSTKSZ=3491 $ ./a.out Also, on the second machine, without this patch: $ gcc tst-minsigstksz-2.c -DMY_MINSIGSTKSZ=3191 $ ./a.out a.out: changed byte 319 bytes below configured stack But with this patch, it gave segfault with a too-small size: $ gcc tst-minsigstksz-2.c -DMY_MINSIGSTKSZ=3191 $ ./a.out Segmentation fault (core dumped) Thanks, Chang
Re: [PATCH v7 5/6] x86/signal: Detect and prevent an alternate signal stack overflow
On Mon, Mar 15, 2021 at 11:52:14PM -0700, Chang S. Bae wrote: > @@ -272,7 +275,8 @@ get_sigframe(struct k_sigaction *ka, struct pt_regs > *regs, size_t frame_size, >* If we are on the alternate signal stack and would overflow it, don't. >* Return an always-bogus address instead so we will die with SIGSEGV. >*/ > - if (onsigstack && !likely(on_sig_stack(sp))) > + if (onsigstack && unlikely(sp <= current->sas_ss_sp || > +sp - current->sas_ss_sp > > current->sas_ss_size)) > return (void __user *)-1L; So clearly I'm missing something because trying to trigger the test case in the bugzilla: https://bugzilla.kernel.org/show_bug.cgi?id=153531 on current tip/master doesn't work. Runs with MY_MINSIGSTKSZ under 2048 fail with: tst-minsigstksz-2: sigaltstack: Cannot allocate memory and above 2048 don't overwrite bytes below the stack. So something else is missing. How did you test this patch? Thx. -- Regards/Gruss, Boris. SUSE Software Solutions Germany GmbH, GF: Felix Imendörffer, HRB 36809, AG Nürnberg
