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

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

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.