Re: [PATCH] stack-protector: Check stack canary for noreturn function

["H.J. Lu" <hjl.tools@gmail.com>]

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.