diff mbox series

[v2,3/4] x86/signal: Prevent an alternate stack overflow before a signal delivery

Message ID 20201119190237.626-4-chang.seok.bae@intel.com
State Not applicable
Headers
Series x86: Improve Minimum Alternate Stack Size |

Commit Message

Chang S. Bae Nov. 19, 2020, 7:02 p.m. UTC 
  The kernel pushes data on the userspace stack when entering a signal. If
using a sigaltstack(), the kernel precisely knows the user stack size.

When the kernel knows that the user stack is too small, avoid the overflow
and do an immediate SIGSEGV instead.

This overflow is known to occur on systems with large XSAVE state. The
effort to increase the size typically used for altstacks reduces the
frequency of these overflows, but this approach is still useful for legacy
binaries.

Here the kernel expects a bit conservative stack size (for 64-bit apps).
Legacy binaries used a too-small sigaltstack would be already overflowed
before this change, if they run on modern hardware.

Signed-off-by: Chang S. Bae <chang.seok.bae@intel.com>
Reviewed-by: Len Brown <len.brown@intel.com>
Cc: x86@kernel.org
Cc: linux-kernel@vger.kernel.org
---
 arch/x86/ia32/ia32_signal.c     | 11 ++++++++---
 arch/x86/include/asm/sigframe.h |  2 ++
 arch/x86/kernel/signal.c        | 16 +++++++++++++++-
 3 files changed, 25 insertions(+), 4 deletions(-)

Comments

Jann Horn Nov. 20, 2020, 11:04 p.m. UTC | #1 
On Thu, Nov 19, 2020 at 8:40 PM Chang S. Bae <chang.seok.bae@intel.com> wrote:
> The kernel pushes data on the userspace stack when entering a signal. If
> using a sigaltstack(), the kernel precisely knows the user stack size.
>
> When the kernel knows that the user stack is too small, avoid the overflow
> and do an immediate SIGSEGV instead.
>
> This overflow is known to occur on systems with large XSAVE state. The
> effort to increase the size typically used for altstacks reduces the
> frequency of these overflows, but this approach is still useful for legacy
> binaries.
>
> Here the kernel expects a bit conservative stack size (for 64-bit apps).
> Legacy binaries used a too-small sigaltstack would be already overflowed
> before this change, if they run on modern hardware.
[...]
> diff --git a/arch/x86/kernel/signal.c b/arch/x86/kernel/signal.c
> index ee6f1ceaa7a2..cee41d684dc2 100644
> --- a/arch/x86/kernel/signal.c
> +++ b/arch/x86/kernel/signal.c
> @@ -251,8 +251,13 @@ 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) {
> +                       /* If the altstack might overflow, die with SIGSEGV: */
> +                       if (!altstack_size_ok(current))
> +                               return (void __user *)-1L;
> +
>                         sp = current->sas_ss_sp + current->sas_ss_size;
> +               }

A couple lines further down, we have this (since commit 14fc9fbc700d):

        /*
         * 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)))
                return (void __user *)-1L;

Is that not working?


(It won't handle the case where the kernel fills up almost all of the
alternate stack, and the userspace signal handler then overflows out
of the alternate signal stack. But there isn't much the kernel can do
about that...)
develop--- via Libc-alpha Nov. 24, 2020, 6:22 p.m. UTC | #2 
> On Nov 20, 2020, at 15:04, Jann Horn <jannh@google.com> wrote:
> 
> On Thu, Nov 19, 2020 at 8:40 PM Chang S. Bae <chang.seok.bae@intel.com> wrote:
>> 
>> diff --git a/arch/x86/kernel/signal.c b/arch/x86/kernel/signal.c
>> index ee6f1ceaa7a2..cee41d684dc2 100644
>> --- a/arch/x86/kernel/signal.c
>> +++ b/arch/x86/kernel/signal.c
>> @@ -251,8 +251,13 @@ 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) {
>> +                       /* If the altstack might overflow, die with SIGSEGV: */
>> +                       if (!altstack_size_ok(current))
>> +                               return (void __user *)-1L;
>> +
>>                        sp = current->sas_ss_sp + current->sas_ss_size;
>> +               }
> 
> A couple lines further down, we have this (since commit 14fc9fbc700d):
> 
>        /*
>         * 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)))
>                return (void __user *)-1L;
> 
> Is that not working?

onsigstack is set at the beginning here. If a signal hits under normal stack,
this flag is not set. Then it will miss the overflow.

The added check allows to detect the sigaltstack overflow (always).

Thanks,
Chang
Jann Horn Nov. 24, 2020, 6:41 p.m. UTC | #3 
On Tue, Nov 24, 2020 at 7:22 PM Bae, Chang Seok
<chang.seok.bae@intel.com> wrote:
> > On Nov 20, 2020, at 15:04, Jann Horn <jannh@google.com> wrote:
> > On Thu, Nov 19, 2020 at 8:40 PM Chang S. Bae <chang.seok.bae@intel.com> wrote:
> >>
> >> diff --git a/arch/x86/kernel/signal.c b/arch/x86/kernel/signal.c
> >> index ee6f1ceaa7a2..cee41d684dc2 100644
> >> --- a/arch/x86/kernel/signal.c
> >> +++ b/arch/x86/kernel/signal.c
> >> @@ -251,8 +251,13 @@ 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) {
> >> +                       /* If the altstack might overflow, die with SIGSEGV: */
> >> +                       if (!altstack_size_ok(current))
> >> +                               return (void __user *)-1L;
> >> +
> >>                        sp = current->sas_ss_sp + current->sas_ss_size;
> >> +               }
> >
> > A couple lines further down, we have this (since commit 14fc9fbc700d):
> >
> >        /*
> >         * 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)))
> >                return (void __user *)-1L;
> >
> > Is that not working?
>
> onsigstack is set at the beginning here. If a signal hits under normal stack,
> this flag is not set. Then it will miss the overflow.
>
> The added check allows to detect the sigaltstack overflow (always).

Ah, I think I understand what you're trying to do. But wouldn't the
better approach be to ensure that the existing on_sig_stack() check is
also used if we just switched to the signal stack? Something like:

diff --git a/arch/x86/kernel/signal.c b/arch/x86/kernel/signal.c
index be0d7d4152ec..2f57842fb4d6 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,10 @@ 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;
+                       onsigstack = true;
+               }
        } else if (IS_ENABLED(CONFIG_X86_32) &&
                   !onsigstack &&
                   regs->ss != __USER_DS &&
develop--- via Libc-alpha Nov. 24, 2020, 8:43 p.m. UTC | #4 
> On Nov 24, 2020, at 10:41, Jann Horn <jannh@google.com> wrote:
> 
> On Tue, Nov 24, 2020 at 7:22 PM Bae, Chang Seok
> <chang.seok.bae@intel.com> wrote:
>>> On Nov 20, 2020, at 15:04, Jann Horn <jannh@google.com> wrote:
>>> On Thu, Nov 19, 2020 at 8:40 PM Chang S. Bae <chang.seok.bae@intel.com> wrote:
>>>> 
>>>> diff --git a/arch/x86/kernel/signal.c b/arch/x86/kernel/signal.c
>>>> index ee6f1ceaa7a2..cee41d684dc2 100644
>>>> --- a/arch/x86/kernel/signal.c
>>>> +++ b/arch/x86/kernel/signal.c
>>>> @@ -251,8 +251,13 @@ 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) {
>>>> +                       /* If the altstack might overflow, die with SIGSEGV: */
>>>> +                       if (!altstack_size_ok(current))
>>>> +                               return (void __user *)-1L;
>>>> +
>>>>                       sp = current->sas_ss_sp + current->sas_ss_size;
>>>> +               }
>>> 
>>> A couple lines further down, we have this (since commit 14fc9fbc700d):
>>> 
>>>       /*
>>>        * 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)))
>>>               return (void __user *)-1L;
>>> 
>>> Is that not working?
>> 
>> onsigstack is set at the beginning here. If a signal hits under normal stack,
>> this flag is not set. Then it will miss the overflow.
>> 
>> The added check allows to detect the sigaltstack overflow (always).
> 
> Ah, I think I understand what you're trying to do. But wouldn't the
> better approach be to ensure that the existing on_sig_stack() check is
> also used if we just switched to the signal stack? Something like:
> 
> diff --git a/arch/x86/kernel/signal.c b/arch/x86/kernel/signal.c
> index be0d7d4152ec..2f57842fb4d6 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,10 @@ 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;
> +                       onsigstack = true;
> +               }
>        } else if (IS_ENABLED(CONFIG_X86_32) &&
>                   !onsigstack &&
>                   regs->ss != __USER_DS &&

Yeah, but wouldn't it better to avoid overwriting user data if we can? The old
check raises segfault *after* overwritten.

The old check is still helpful to detect an overflow from the nested signal(s)
under sigaltstack.

Thanks,
Chang
Jann Horn Nov. 24, 2020, 8:47 p.m. UTC | #5 
On Tue, Nov 24, 2020 at 9:43 PM Bae, Chang Seok
<chang.seok.bae@intel.com> wrote:
> > On Nov 24, 2020, at 10:41, Jann Horn <jannh@google.com> wrote:
> > On Tue, Nov 24, 2020 at 7:22 PM Bae, Chang Seok
> > <chang.seok.bae@intel.com> wrote:
> >>> On Nov 20, 2020, at 15:04, Jann Horn <jannh@google.com> wrote:
> >>> On Thu, Nov 19, 2020 at 8:40 PM Chang S. Bae <chang.seok.bae@intel.com> wrote:
> >>>>
> >>>> diff --git a/arch/x86/kernel/signal.c b/arch/x86/kernel/signal.c
> >>>> index ee6f1ceaa7a2..cee41d684dc2 100644
> >>>> --- a/arch/x86/kernel/signal.c
> >>>> +++ b/arch/x86/kernel/signal.c
> >>>> @@ -251,8 +251,13 @@ 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) {
> >>>> +                       /* If the altstack might overflow, die with SIGSEGV: */
> >>>> +                       if (!altstack_size_ok(current))
> >>>> +                               return (void __user *)-1L;
> >>>> +
> >>>>                       sp = current->sas_ss_sp + current->sas_ss_size;
> >>>> +               }
> >>>
> >>> A couple lines further down, we have this (since commit 14fc9fbc700d):
> >>>
> >>>       /*
> >>>        * 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)))
> >>>               return (void __user *)-1L;
> >>>
> >>> Is that not working?
> >>
> >> onsigstack is set at the beginning here. If a signal hits under normal stack,
> >> this flag is not set. Then it will miss the overflow.
> >>
> >> The added check allows to detect the sigaltstack overflow (always).
> >
> > Ah, I think I understand what you're trying to do. But wouldn't the
> > better approach be to ensure that the existing on_sig_stack() check is
> > also used if we just switched to the signal stack? Something like:
> >
> > diff --git a/arch/x86/kernel/signal.c b/arch/x86/kernel/signal.c
> > index be0d7d4152ec..2f57842fb4d6 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,10 @@ 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;
> > +                       onsigstack = true;
> > +               }
> >        } else if (IS_ENABLED(CONFIG_X86_32) &&
> >                   !onsigstack &&
> >                   regs->ss != __USER_DS &&
>
> Yeah, but wouldn't it better to avoid overwriting user data if we can? The old
> check raises segfault *after* overwritten.

Where is that overwrite happening? Between the point where your check
happens, and the point where the old check is, the only calls are to
fpu__alloc_mathframe() and align_sigframe(), right?
fpu__alloc_mathframe() just does some size calculations and doesn't
write anything. align_sigframe() also just does size calculations. Am
I missing something?
develop--- via Libc-alpha Nov. 24, 2020, 8:55 p.m. UTC | #6 
> On Nov 24, 2020, at 12:47, Jann Horn <jannh@google.com> wrote:
> 
> On Tue, Nov 24, 2020 at 9:43 PM Bae, Chang Seok
> <chang.seok.bae@intel.com> wrote:
>>> On Nov 24, 2020, at 10:41, Jann Horn <jannh@google.com> wrote:
>>> On Tue, Nov 24, 2020 at 7:22 PM Bae, Chang Seok
>>> <chang.seok.bae@intel.com> wrote:
>>>>> On Nov 20, 2020, at 15:04, Jann Horn <jannh@google.com> wrote:
>>>>> On Thu, Nov 19, 2020 at 8:40 PM Chang S. Bae <chang.seok.bae@intel.com> wrote:
>>>>>> 
>>>>>> diff --git a/arch/x86/kernel/signal.c b/arch/x86/kernel/signal.c
>>>>>> index ee6f1ceaa7a2..cee41d684dc2 100644
>>>>>> --- a/arch/x86/kernel/signal.c
>>>>>> +++ b/arch/x86/kernel/signal.c
>>>>>> @@ -251,8 +251,13 @@ 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) {
>>>>>> +                       /* If the altstack might overflow, die with SIGSEGV: */
>>>>>> +                       if (!altstack_size_ok(current))
>>>>>> +                               return (void __user *)-1L;
>>>>>> +
>>>>>>                      sp = current->sas_ss_sp + current->sas_ss_size;
>>>>>> +               }
>>>>> 
>>>>> A couple lines further down, we have this (since commit 14fc9fbc700d):
>>>>> 
>>>>>      /*
>>>>>       * 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)))
>>>>>              return (void __user *)-1L;
>>>>> 
>>>>> Is that not working?
>>>> 
>>>> onsigstack is set at the beginning here. If a signal hits under normal stack,
>>>> this flag is not set. Then it will miss the overflow.
>>>> 
>>>> The added check allows to detect the sigaltstack overflow (always).
>>> 
>>> Ah, I think I understand what you're trying to do. But wouldn't the
>>> better approach be to ensure that the existing on_sig_stack() check is
>>> also used if we just switched to the signal stack? Something like:
>>> 
>>> diff --git a/arch/x86/kernel/signal.c b/arch/x86/kernel/signal.c
>>> index be0d7d4152ec..2f57842fb4d6 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,10 @@ 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;
>>> +                       onsigstack = true;
>>> +               }
>>>       } else if (IS_ENABLED(CONFIG_X86_32) &&
>>>                  !onsigstack &&
>>>                  regs->ss != __USER_DS &&
>> 
>> Yeah, but wouldn't it better to avoid overwriting user data if we can? The old
>> check raises segfault *after* overwritten.
> 
> Where is that overwrite happening? Between the point where your check
> happens, and the point where the old check is, the only calls are to
> fpu__alloc_mathframe() and align_sigframe(), right?
> fpu__alloc_mathframe() just does some size calculations and doesn't
> write anything. align_sigframe() also just does size calculations. Am
> I missing something?

Yeah, you’re right. Right now, I’m thinking your approach is simpler and
providing almost the same function (unless I’m missing here).

Thanks,
Chang
develop--- via Libc-alpha Feb. 8, 2021, 8:29 p.m. UTC | #7 
On Nov 24, 2020, at 10:41, Jann Horn <jannh@google.com> wrote:
> On Tue, Nov 24, 2020 at 7:22 PM Bae, Chang Seok
> <chang.seok.bae@intel.com> wrote:
>>> On Nov 20, 2020, at 15:04, Jann Horn <jannh@google.com> wrote:
>>> On Thu, Nov 19, 2020 at 8:40 PM Chang S. Bae <chang.seok.bae@intel.com> wrote:
>>>> 
>>>> diff --git a/arch/x86/kernel/signal.c b/arch/x86/kernel/signal.c
>>>> index ee6f1ceaa7a2..cee41d684dc2 100644
>>>> --- a/arch/x86/kernel/signal.c
>>>> +++ b/arch/x86/kernel/signal.c
>>>> @@ -251,8 +251,13 @@ 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) {
>>>> +                       /* If the altstack might overflow, die with SIGSEGV: */
>>>> +                       if (!altstack_size_ok(current))
>>>> +                               return (void __user *)-1L;
>>>> +
>>>>                       sp = current->sas_ss_sp + current->sas_ss_size;
>>>> +               }
>>> 
>>> A couple lines further down, we have this (since commit 14fc9fbc700d):
>>> 
>>>       /*
>>>        * 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)))
>>>               return (void __user *)-1L;
>>> 
>>> Is that not working?
>> 
>> onsigstack is set at the beginning here. If a signal hits under normal stack,
>> this flag is not set. Then it will miss the overflow.
>> 
>> The added check allows to detect the sigaltstack overflow (always).
> 
> Ah, I think I understand what you're trying to do. But wouldn't the
> better approach be to ensure that the existing on_sig_stack() check is
> also used if we just switched to the signal stack? Something like:
> 
> diff --git a/arch/x86/kernel/signal.c b/arch/x86/kernel/signal.c
> index be0d7d4152ec..2f57842fb4d6 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,10 @@ 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;
> +                       onsigstack = true;

FWIW, here. 

Thanks to the report by Oliver via the kernel test robot, I realized that
this needs to be conditional on the SS_AUTODISARM tag like, :

    onsigstack = !(current->sas_ss_flags & SS_AUTODISARM);

Thanks,
Chang
diff mbox series

Patch

diff --git a/arch/x86/ia32/ia32_signal.c b/arch/x86/ia32/ia32_signal.c
index 81cf22398cd1..85abd9eb79d5 100644
--- a/arch/x86/ia32/ia32_signal.c
+++ b/arch/x86/ia32/ia32_signal.c
@@ -210,13 +210,18 @@  static void __user *get_sigframe(struct ksignal *ksig, struct pt_regs *regs,
 	sp = regs->sp;
 
 	/* This is the X/Open sanctioned signal stack switching.  */
-	if (ksig->ka.sa.sa_flags & SA_ONSTACK)
+	if (ksig->ka.sa.sa_flags & SA_ONSTACK) {
+		/* If the altstack might overflow, die with SIGSEGV: */
+		if (!altstack_size_ok(current))
+			return (void __user *)-1L;
+
 		sp = sigsp(sp, ksig);
 	/* This is the legacy signal stack switching. */
-	else if (regs->ss != __USER32_DS &&
+	} else if (regs->ss != __USER32_DS &&
 		!(ksig->ka.sa.sa_flags & SA_RESTORER) &&
-		 ksig->ka.sa.sa_restorer)
+		 ksig->ka.sa.sa_restorer) {
 		sp = (unsigned long) ksig->ka.sa.sa_restorer;
+	}
 
 	sp = fpu__alloc_mathframe(sp, 1, &fx_aligned, &math_size);
 	*fpstate = (struct _fpstate_32 __user *) sp;
diff --git a/arch/x86/include/asm/sigframe.h b/arch/x86/include/asm/sigframe.h
index ac77f3f90bc9..c9f2f9ace76f 100644
--- a/arch/x86/include/asm/sigframe.h
+++ b/arch/x86/include/asm/sigframe.h
@@ -106,6 +106,8 @@  struct rt_sigframe_x32 {
 #define SIZEOF_rt_sigframe_x32	0
 #endif
 
+bool altstack_size_ok(struct task_struct *tsk);
+
 void __init init_sigframe_size(void);
 
 #endif /* _ASM_X86_SIGFRAME_H */
diff --git a/arch/x86/kernel/signal.c b/arch/x86/kernel/signal.c
index ee6f1ceaa7a2..cee41d684dc2 100644
--- a/arch/x86/kernel/signal.c
+++ b/arch/x86/kernel/signal.c
@@ -251,8 +251,13 @@  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) {
+			/* If the altstack might overflow, die with SIGSEGV: */
+			if (!altstack_size_ok(current))
+				return (void __user *)-1L;
+
 			sp = current->sas_ss_sp + current->sas_ss_size;
+		}
 	} else if (IS_ENABLED(CONFIG_X86_32) &&
 		   !onsigstack &&
 		   regs->ss != __USER_DS &&
@@ -725,6 +730,15 @@  unsigned long get_sigframe_size(void)
 	return max_frame_size;
 }
 
+bool altstack_size_ok(struct task_struct *tsk)
+{
+	/*
+	 * Can this task's sigaltstack accommodate the largest
+	 * signal frame the kernel might need?
+	 */
+	return (tsk->sas_ss_size >= max_frame_size);
+}
+
 static inline int is_ia32_compat_frame(struct ksignal *ksig)
 {
 	return IS_ENABLED(CONFIG_IA32_EMULATION) &&