On Tue, Aug 2, 2022 at 4:34 PM Jeff Law <[email protected]> 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
> > <[email protected]> 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.
