* [PATCH] stack-protector: Check stack canary for noreturn function
@ 2022-07-14 21:55 H.J. Lu
2022-07-30 20:30 ` Jeff Law
0 siblings, 1 reply; 6+ messages in thread
From: H.J. Lu @ 2022-07-14 21:55 UTC (permalink / raw)
To: gcc-patches; +Cc: Jakub Jelinek
Check stack canary for noreturn function to catch stack corruption
before calling noreturn function. For C++, check stack canary when
throwing exception or resuming stack unwind to avoid corrupted stack.
gcc/
PR middle-end/58245
* calls.cc (expand_call): Check stack canary for noreturn
function.
gcc/testsuite/
PR middle-end/58245
* c-c++-common/pr58245-1.c: New test.
* g++.dg/pr58245-1.C: Likewise.
* g++.dg/fstack-protector-strong.C: Adjusted.
---
gcc/calls.cc | 7 ++++++-
gcc/testsuite/c-c++-common/pr58245-1.c | 12 ++++++++++++
gcc/testsuite/g++.dg/fstack-protector-strong.C | 2 +-
gcc/testsuite/g++.dg/pr58245-1.C | 10 ++++++++++
4 files changed, 29 insertions(+), 2 deletions(-)
create mode 100644 gcc/testsuite/c-c++-common/pr58245-1.c
create mode 100644 gcc/testsuite/g++.dg/pr58245-1.C
diff --git a/gcc/calls.cc b/gcc/calls.cc
index bc96aff38f0..7816c2c8d99 100644
--- a/gcc/calls.cc
+++ b/gcc/calls.cc
@@ -3154,7 +3154,12 @@ expand_call (tree exp, rtx target, int ignore)
if (pass && (flags & ECF_MALLOC))
start_sequence ();
- if (pass == 0
+ /* Check the canary value for sibcall or function which doesn't
+ return. */
+ if ((pass == 0
+ || ((flags & ECF_NORETURN) != 0
+ && (fndecl
+ != get_callee_fndecl (targetm.stack_protect_fail ()))))
&& crtl->stack_protect_guard
&& targetm.stack_protect_runtime_enabled_p ())
stack_protect_epilogue ();
diff --git a/gcc/testsuite/c-c++-common/pr58245-1.c b/gcc/testsuite/c-c++-common/pr58245-1.c
new file mode 100644
index 00000000000..945acc53004
--- /dev/null
+++ b/gcc/testsuite/c-c++-common/pr58245-1.c
@@ -0,0 +1,12 @@
+/* { dg-do compile { target i?86-*-* x86_64-*-* rs6000-*-* s390x-*-* } } */
+/* { dg-options "-O2 -fstack-protector-all" } */
+
+extern void foo (void) __attribute__ ((noreturn));
+
+void
+bar (void)
+{
+ foo ();
+}
+
+/* { dg-final { scan-assembler-times "stack_chk_fail" 1 } } */
diff --git a/gcc/testsuite/g++.dg/fstack-protector-strong.C b/gcc/testsuite/g++.dg/fstack-protector-strong.C
index ae6d2fdb8df..034af2ce9ab 100644
--- a/gcc/testsuite/g++.dg/fstack-protector-strong.C
+++ b/gcc/testsuite/g++.dg/fstack-protector-strong.C
@@ -85,4 +85,4 @@ int foo7 (B *p)
return p->return_slot ().a1;
}
-/* { dg-final { scan-assembler-times "stack_chk_fail" 7 } } */
+/* { dg-final { scan-assembler-times "stack_chk_fail" 8 } } */
diff --git a/gcc/testsuite/g++.dg/pr58245-1.C b/gcc/testsuite/g++.dg/pr58245-1.C
new file mode 100644
index 00000000000..1439bc62e71
--- /dev/null
+++ b/gcc/testsuite/g++.dg/pr58245-1.C
@@ -0,0 +1,10 @@
+/* { dg-do compile { target i?86-*-* x86_64-*-* rs6000-*-* s390x-*-* } } */
+/* { dg-options "-O2 -fstack-protector-all" } */
+
+void
+bar (void)
+{
+ throw 1;
+}
+
+/* { dg-final { scan-assembler-times "stack_chk_fail" 1 } } */
--
2.36.1
^ permalink raw reply [flat|nested] 6+ messages in thread
* Re: [PATCH] stack-protector: Check stack canary for noreturn function
2022-07-14 21:55 [PATCH] stack-protector: Check stack canary for noreturn function H.J. Lu
@ 2022-07-30 20:30 ` Jeff Law
2022-08-02 17:43 ` H.J. Lu
0 siblings, 1 reply; 6+ messages in thread
From: Jeff Law @ 2022-07-30 20:30 UTC (permalink / raw)
To: gcc-patches
On 7/14/2022 3:55 PM, H.J. Lu via Gcc-patches wrote:
> Check stack canary for noreturn function to catch stack corruption
> before calling noreturn function. For C++, check stack canary when
> throwing exception or resuming stack unwind to avoid corrupted stack.
>
> gcc/
>
> PR middle-end/58245
> * calls.cc (expand_call): Check stack canary for noreturn
> function.
>
> gcc/testsuite/
>
> PR middle-end/58245
> * c-c++-common/pr58245-1.c: New test.
> * g++.dg/pr58245-1.C: Likewise.
> * g++.dg/fstack-protector-strong.C: Adjusted.
But is this really something we want? I'd actually lean towards
eliminating the useless load -- I don't necessarily think we should be
treating non-returning paths specially here.
The whole point of the stack protector is to prevent the *return* path
from going to an attacker controlled location. I'm not sure checking
the protector at this point actually does anything particularly useful.
jeff
^ permalink raw reply [flat|nested] 6+ messages in thread
* Re: [PATCH] stack-protector: Check stack canary for noreturn function
2022-07-30 20:30 ` Jeff Law
@ 2022-08-02 17:43 ` H.J. Lu
2022-08-02 23:34 ` Jeff Law
0 siblings, 1 reply; 6+ messages in thread
From: H.J. Lu @ 2022-08-02 17:43 UTC (permalink / raw)
To: Jeff Law; +Cc: GCC Patches
On Sat, Jul 30, 2022 at 1:30 PM Jeff Law via Gcc-patches
<gcc-patches@gcc.gnu.org> wrote:
>
>
>
> On 7/14/2022 3:55 PM, H.J. Lu via Gcc-patches wrote:
> > Check stack canary for noreturn function to catch stack corruption
> > before calling noreturn function. For C++, check stack canary when
> > throwing exception or resuming stack unwind to avoid corrupted stack.
> >
> > gcc/
> >
> > PR middle-end/58245
> > * calls.cc (expand_call): Check stack canary for noreturn
> > function.
> >
> > gcc/testsuite/
> >
> > PR middle-end/58245
> > * c-c++-common/pr58245-1.c: New test.
> > * g++.dg/pr58245-1.C: Likewise.
> > * g++.dg/fstack-protector-strong.C: Adjusted.
> But is this really something we want? I'd actually lean towards
> eliminating the useless load -- I don't necessarily think we should be
> treating non-returning paths specially here.
>
> The whole point of the stack protector is to prevent the *return* path
> from going to an attacker controlled location. I'm not sure checking
> the protector at this point actually does anything particularly useful.
throw is marked no return. Since the unwind library may read
the stack contents to unwind stack, it the stack is corrupted, the
exception handling may go wrong. Should we handle this case?
--
H.J.
^ permalink raw reply [flat|nested] 6+ messages in thread
* Re: [PATCH] stack-protector: Check stack canary for noreturn function
2022-08-02 17:43 ` H.J. Lu
@ 2022-08-02 23:34 ` Jeff Law
2022-08-03 17:27 ` H.J. Lu
0 siblings, 1 reply; 6+ messages in thread
From: Jeff Law @ 2022-08-02 23:34 UTC (permalink / raw)
To: H.J. Lu; +Cc: GCC Patches
On 8/2/2022 11:43 AM, H.J. Lu wrote:
> On Sat, Jul 30, 2022 at 1:30 PM Jeff Law via Gcc-patches
> <gcc-patches@gcc.gnu.org> wrote:
>>
>>
>> On 7/14/2022 3:55 PM, H.J. Lu via Gcc-patches wrote:
>>> Check stack canary for noreturn function to catch stack corruption
>>> before calling noreturn function. For C++, check stack canary when
>>> throwing exception or resuming stack unwind to avoid corrupted stack.
>>>
>>> gcc/
>>>
>>> PR middle-end/58245
>>> * calls.cc (expand_call): Check stack canary for noreturn
>>> function.
>>>
>>> gcc/testsuite/
>>>
>>> PR middle-end/58245
>>> * c-c++-common/pr58245-1.c: New test.
>>> * g++.dg/pr58245-1.C: Likewise.
>>> * g++.dg/fstack-protector-strong.C: Adjusted.
>> But is this really something we want? I'd actually lean towards
>> eliminating the useless load -- I don't necessarily think we should be
>> treating non-returning paths specially here.
>>
>> The whole point of the stack protector is to prevent the *return* path
>> from going to an attacker controlled location. I'm not sure checking
>> the protector at this point actually does anything particularly useful.
> throw is marked no return. Since the unwind library may read
> the stack contents to unwind stack, it the stack is corrupted, the
> exception handling may go wrong. Should we handle this case?
That's the question I think we need to answer. The EH paths are a known
security issue on Windows and while ours are notably different I'm not
sure if there's a real attack surface in those paths. My sense is that
if we need to tackle this that doing so on the throw side might be
better as it's closer conceptually to when//how we check the canary for
a normal return.
jeff
>
> --
> H.J.
^ permalink raw reply [flat|nested] 6+ messages in thread
* Re: [PATCH] stack-protector: Check stack canary for noreturn function
2022-08-02 23:34 ` Jeff Law
@ 2022-08-03 17:27 ` H.J. Lu
2022-08-17 22:19 ` H.J. Lu
0 siblings, 1 reply; 6+ messages in thread
From: H.J. Lu @ 2022-08-03 17:27 UTC (permalink / raw)
To: Jeff Law; +Cc: GCC Patches
On Tue, Aug 2, 2022 at 4:34 PM Jeff Law <jeffreyalaw@gmail.com> wrote:
>
>
>
> On 8/2/2022 11:43 AM, H.J. Lu wrote:
> > On Sat, Jul 30, 2022 at 1:30 PM Jeff Law via Gcc-patches
> > <gcc-patches@gcc.gnu.org> wrote:
> >>
> >>
> >> On 7/14/2022 3:55 PM, H.J. Lu via Gcc-patches wrote:
> >>> Check stack canary for noreturn function to catch stack corruption
> >>> before calling noreturn function. For C++, check stack canary when
> >>> throwing exception or resuming stack unwind to avoid corrupted stack.
> >>>
> >>> gcc/
> >>>
> >>> PR middle-end/58245
> >>> * calls.cc (expand_call): Check stack canary for noreturn
> >>> function.
> >>>
> >>> gcc/testsuite/
> >>>
> >>> PR middle-end/58245
> >>> * c-c++-common/pr58245-1.c: New test.
> >>> * g++.dg/pr58245-1.C: Likewise.
> >>> * g++.dg/fstack-protector-strong.C: Adjusted.
> >> But is this really something we want? I'd actually lean towards
> >> eliminating the useless load -- I don't necessarily think we should be
> >> treating non-returning paths specially here.
> >>
> >> The whole point of the stack protector is to prevent the *return* path
> >> from going to an attacker controlled location. I'm not sure checking
> >> the protector at this point actually does anything particularly useful.
> > throw is marked no return. Since the unwind library may read
> > the stack contents to unwind stack, it the stack is corrupted, the
> > exception handling may go wrong. Should we handle this case?
> That's the question I think we need to answer. The EH paths are a known
> security issue on Windows and while ours are notably different I'm not
> sure if there's a real attack surface in those paths. My sense is that
> if we need to tackle this that doing so on the throw side might be
> better as it's closer conceptually to when//how we check the canary for
> a normal return.
Like this?
@@ -3154,7 +3155,10 @@ expand_call (tree exp, rtx target, int ignore)
if (pass && (flags & ECF_MALLOC))
start_sequence ();
- if (pass == 0
+ /* Check the canary value for sibcall or function which doesn't
+ return and could throw. */
+ if ((pass == 0
+ || ((flags & ECF_NORETURN) != 0 && tree_could_throw_p (exp)))
&& crtl->stack_protect_guard
&& targetm.stack_protect_runtime_enabled_p ())
stack_protect_epilogue ();
> jeff
> >
> > --
> > H.J.
>
--
H.J.
^ permalink raw reply [flat|nested] 6+ messages in thread
* Re: [PATCH] stack-protector: Check stack canary for noreturn function
2022-08-03 17:27 ` H.J. Lu
@ 2022-08-17 22:19 ` H.J. Lu
0 siblings, 0 replies; 6+ messages in thread
From: H.J. Lu @ 2022-08-17 22:19 UTC (permalink / raw)
To: Jeff Law; +Cc: GCC Patches
On Wed, Aug 3, 2022 at 10:27 AM H.J. Lu <hjl.tools@gmail.com> wrote:
>
> On Tue, Aug 2, 2022 at 4:34 PM Jeff Law <jeffreyalaw@gmail.com> wrote:
> >
> >
> >
> > On 8/2/2022 11:43 AM, H.J. Lu wrote:
> > > On Sat, Jul 30, 2022 at 1:30 PM Jeff Law via Gcc-patches
> > > <gcc-patches@gcc.gnu.org> wrote:
> > >>
> > >>
> > >> On 7/14/2022 3:55 PM, H.J. Lu via Gcc-patches wrote:
> > >>> Check stack canary for noreturn function to catch stack corruption
> > >>> before calling noreturn function. For C++, check stack canary when
> > >>> throwing exception or resuming stack unwind to avoid corrupted stack.
> > >>>
> > >>> gcc/
> > >>>
> > >>> PR middle-end/58245
> > >>> * calls.cc (expand_call): Check stack canary for noreturn
> > >>> function.
> > >>>
> > >>> gcc/testsuite/
> > >>>
> > >>> PR middle-end/58245
> > >>> * c-c++-common/pr58245-1.c: New test.
> > >>> * g++.dg/pr58245-1.C: Likewise.
> > >>> * g++.dg/fstack-protector-strong.C: Adjusted.
> > >> But is this really something we want? I'd actually lean towards
> > >> eliminating the useless load -- I don't necessarily think we should be
> > >> treating non-returning paths specially here.
> > >>
> > >> The whole point of the stack protector is to prevent the *return* path
> > >> from going to an attacker controlled location. I'm not sure checking
> > >> the protector at this point actually does anything particularly useful.
> > > throw is marked no return. Since the unwind library may read
> > > the stack contents to unwind stack, it the stack is corrupted, the
> > > exception handling may go wrong. Should we handle this case?
> > That's the question I think we need to answer. The EH paths are a known
> > security issue on Windows and while ours are notably different I'm not
> > sure if there's a real attack surface in those paths. My sense is that
> > if we need to tackle this that doing so on the throw side might be
> > better as it's closer conceptually to when//how we check the canary for
> > a normal return.
>
> Like this?
>
> @@ -3154,7 +3155,10 @@ expand_call (tree exp, rtx target, int ignore)
> if (pass && (flags & ECF_MALLOC))
> start_sequence ();
>
> - if (pass == 0
> + /* Check the canary value for sibcall or function which doesn't
> + return and could throw. */
> + if ((pass == 0
> + || ((flags & ECF_NORETURN) != 0 && tree_could_throw_p (exp)))
> && crtl->stack_protect_guard
> && targetm.stack_protect_runtime_enabled_p ())
> stack_protect_epilogue ();
Here is the patch:
https://gcc.gnu.org/pipermail/gcc-patches/2022-August/599916.html
> > jeff
> > >
> > > --
> > > H.J.
> >
>
>
> --
> H.J.
--
H.J.
^ permalink raw reply [flat|nested] 6+ messages in thread
end of thread, other threads:[~2022-08-17 22:20 UTC | newest]
Thread overview: 6+ messages (download: mbox.gz / follow: Atom feed)
-- links below jump to the message on this page --
2022-07-14 21:55 [PATCH] stack-protector: Check stack canary for noreturn function H.J. Lu
2022-07-30 20:30 ` Jeff Law
2022-08-02 17:43 ` H.J. Lu
2022-08-02 23:34 ` Jeff Law
2022-08-03 17:27 ` H.J. Lu
2022-08-17 22:19 ` H.J. Lu
This is a public inbox, see mirroring instructions
for how to clone and mirror all data and code used for this inbox;
as well as URLs for read-only IMAP folder(s) and NNTP newsgroup(s).