[Bug target/112470] New: [AARCH64]stack-protector vulnerability fixing solution impact code size and performance.

[Bug target/112470] New: [AARCH64]stack-protector vulnerability fixing solution impact code size and performance.

[dongjianqiang2 at huawei dot com]

https://gcc.gnu.org/bugzilla/show_bug.cgi?id=112470

            Bug ID: 112470
           Summary: [AARCH64]stack-protector vulnerability fixing solution
                    impact code size and performance.
           Product: gcc
           Version: 13.1.0
            Status: UNCONFIRMED
          Severity: normal
          Priority: P3
         Component: target
          Assignee: unassigned at gcc dot gnu.org
          Reporter: dongjianqiang2 at huawei dot com
  Target Milestone: ---

Hi, after the CVE-2023-4039 patch is installed, the code size and performance
are affected after stack protection is enabled. 
Refer to https://godbolt.org/z/7dWeYd5Kb
After analyzing the patch, I think we can add the following modification
solution: For functions that need to dynamically apply for stackframe, we can
change the stack distribution of local variables, in other cases, the original
stack distribution will be retained.

diff --git a/gcc/config/aarch64/aarch64.cc b/gcc/config/aarch64/aarch64.cc
index 800a8b0e110..342bab89281 100644
--- a/gcc/config/aarch64/aarch64.cc
+++ b/gcc/config/aarch64/aarch64.cc
@@ -8538,7 +8538,7 @@ aarch64_save_regs_above_locals_p ()
      it would be possible for a carefully sized smash attack to change
      the saved registers (particularly LR and FP) without reaching the
      canary.  */
-  return crtl->stack_protect_guard;
+  return crtl->stack_protect_guard && cfun->calls_alloca;
 }

 /* Mark the registers that need to be saved by the callee and calculate


Please let me know if you have any comments

[Bug target/112470] [11/12/13/14 regression] [AARCH64] stack-protector vulnerability fixing solution impact code size and performance

[sjames at gcc dot gnu.org]

https://gcc.gnu.org/bugzilla/show_bug.cgi?id=112470

Sam James <sjames at gcc dot gnu.org> changed:

           What    |Removed                     |Added
----------------------------------------------------------------------------
            Summary|[AARCH64] stack-protector   |[11/12/13/14 regression]
                   |vulnerability fixing        |[AARCH64] stack-protector
                   |solution impact code size   |vulnerability fixing
                   |and performance             |solution impact code size
                   |                            |and performance

--- Comment #1 from Sam James <sjames at gcc dot gnu.org> ---
This is I suppose a 11/12/13/14 regression (if valid) given we backported it
all the way down.

[Bug target/112470] [11/12/13/14 regression] [AARCH64] stack-protector vulnerability fixing solution impact code size and performance

[pinskia at gcc dot gnu.org]

https://gcc.gnu.org/bugzilla/show_bug.cgi?id=112470

Andrew Pinski <pinskia at gcc dot gnu.org> changed:

           What    |Removed                     |Added
----------------------------------------------------------------------------
           Severity|normal                      |critical
           Priority|P3                          |P1
           Keywords|                            |missed-optimization
   Target Milestone|---                         |11.5

--- Comment #2 from Andrew Pinski <pinskia at gcc dot gnu.org> ---
(In reply to Sam James from comment #1)
> This is I suppose a 11/12/13/14 regression (if valid) given we backported it
> all the way down.

P1 because it was not in releases before hand too.

[Bug target/112470] [11/12/13/14 regression] [AARCH64] stack-protector vulnerability fixing solution impact code size and performance

[dongjianqiang2 at huawei dot com]

https://gcc.gnu.org/bugzilla/show_bug.cgi?id=112470

--- Comment #3 from John Dong <dongjianqiang2 at huawei dot com> ---
(In reply to John Dong from comment #0)
> Hi, after the CVE-2023-4039 patch is installed, the code size and
> performance are affected after stack protection is enabled. 
> Refer to https://godbolt.org/z/7dWeYd5Kb
> After analyzing the patch, I think we can add the following modification
> solution: For functions that need to dynamically apply for stackframe, we
> can change the stack distribution of local variables, in other cases, the
> original stack distribution will be retained.
> 
> diff --git a/gcc/config/aarch64/aarch64.cc b/gcc/config/aarch64/aarch64.cc
> index 800a8b0e110..342bab89281 100644
> --- a/gcc/config/aarch64/aarch64.cc
> +++ b/gcc/config/aarch64/aarch64.cc
> @@ -8538,7 +8538,7 @@ aarch64_save_regs_above_locals_p ()
>       it would be possible for a carefully sized smash attack to change
>       the saved registers (particularly LR and FP) without reaching the
>       canary.  */
> -  return crtl->stack_protect_guard;
> +  return crtl->stack_protect_guard && cfun->calls_alloca;
>  }
> 
>  /* Mark the registers that need to be saved by the callee and calculate
> 
> 
> Please let me know if you have any comments

ping ?

[Bug target/112470] [11/12/13/14 regression] [AARCH64] stack-protector vulnerability fixing solution impact code size and performance

[sjames at gcc dot gnu.org]

https://gcc.gnu.org/bugzilla/show_bug.cgi?id=112470

--- Comment #4 from Sam James <sjames at gcc dot gnu.org> ---
(In reply to John Dong from comment #3)
> ping ?

You posted the bug after the close of business on Friday (end of the week) and
the next week is just starting now on Monday. Europe is waking up now and the
US hasn't yet. Please wait.

[Bug target/112470] [11/12/13/14 regression] [AARCH64] stack-protector vulnerability fixing solution impact code size and performance

[rsandifo at gcc dot gnu.org]

https://gcc.gnu.org/bugzilla/show_bug.cgi?id=112470

--- Comment #5 from Richard Sandiford <rsandifo at gcc dot gnu.org> ---
Could you quantify the performance impact that you're seeing?  Figures relative
to no protection and to unpatched -fstack-protector-strong would be useful.

-fstack-protector-strong adds a large overhead to small functions like this
[https://godbolt.org/z/x1v7hvsq1].  And the option explicitly asks for
statically sized arrays to be “protected”.  Users who only want variable-length
arrays to be protected would instead use -fstack-protector.

So given that the function has the kind of array that the user wants to be
protected, it seems safer to provide the same kind of protection as other
targets do, in the same way, rather than try to do our own thing.  We've
already been bitten once by this.

For example, on other targets, -fstack-protector-strong effectively protects
against underflow as well as overflow, in the sense that underflow can't
overwrite the function's saved registers.  The proposed patch would change that
for AArch64: writing at negative indices would allow the saved registers to be
overwritten.

[Bug target/112470] [11/12/13/14 regression] [AARCH64] stack-protector vulnerability fixing solution impact code size and performance

[dongjianqiang2 at huawei dot com]

https://gcc.gnu.org/bugzilla/show_bug.cgi?id=112470

--- Comment #6 from John Dong <dongjianqiang2 at huawei dot com> ---
(In reply to Richard Sandiford from comment #5)
> Could you quantify the performance impact that you're seeing?  Figures
> relative to no protection and to unpatched -fstack-protector-strong would be
> useful.
> 
> -fstack-protector-strong adds a large overhead to small functions like this
> [https://godbolt.org/z/x1v7hvsq1].  And the option explicitly asks for
> statically sized arrays to be “protected”.  Users who only want
> variable-length arrays to be protected would instead use -fstack-protector.
> 
> So given that the function has the kind of array that the user wants to be
> protected, it seems safer to provide the same kind of protection as other
> targets do, in the same way, rather than try to do our own thing.  We've
> already been bitten once by this.
> 
> For example, on other targets, -fstack-protector-strong effectively protects
> against underflow as well as overflow, in the sense that underflow can't
> overwrite the function's saved registers.  The proposed patch would change
> that for AArch64: writing at negative indices would allow the saved
> registers to be overwritten.

For applications without stack protection, there is no difference because the
function stack frame not changed when aarch64_save_regs_above_locals_p is
false. I didn't run any spec tests, but I saw about a 0.5% CPU usage increasing
for some applications between patched -fstack-protector-strong and unpatched
-fstack-protector-strong.

[Bug target/112470] [11/12/13/14 regression] [AARCH64] stack-protector vulnerability fixing solution impact code size and performance

[rsandifo at gcc dot gnu.org]

https://gcc.gnu.org/bugzilla/show_bug.cgi?id=112470

--- Comment #7 from Richard Sandiford <rsandifo at gcc dot gnu.org> ---
(In reply to John Dong from comment #6)
> For applications without stack protection, there is no difference because
> the function stack frame not changed when aarch64_save_regs_above_locals_p
> is false. I didn't run any spec tests, but I saw about a 0.5% CPU usage
> increasing for some applications between patched -fstack-protector-strong
> and unpatched -fstack-protector-strong.

Thanks!  For the non-stack-protector comparison: I meant how much overhead do
you see when you enable -fstack-protector-strong with the old compiler vs. not
enabling it?  In other words, I was wondering about the old unprotected vs. old
protected comparison, rather than the old unprotected vs. new unprotected
(which I agree shouldn't change).

[Bug target/112470] [11/12/13/14 regression] [AARCH64] stack-protector vulnerability fixing solution impact code size and performance

[dongjianqiang2 at huawei dot com]

https://gcc.gnu.org/bugzilla/show_bug.cgi?id=112470

--- Comment #8 from John Dong <dongjianqiang2 at huawei dot com> ---
(In reply to Richard Sandiford from comment #7)
> (In reply to John Dong from comment #6)
> > For applications without stack protection, there is no difference because
> > the function stack frame not changed when aarch64_save_regs_above_locals_p
> > is false. I didn't run any spec tests, but I saw about a 0.5% CPU usage
> > increasing for some applications between patched -fstack-protector-strong
> > and unpatched -fstack-protector-strong.
> 
> Thanks!  For the non-stack-protector comparison: I meant how much overhead
> do you see when you enable -fstack-protector-strong with the old compiler
> vs. not enabling it?  In other words, I was wondering about the old
> unprotected vs. old protected comparison, rather than the old unprotected
> vs. new unprotected (which I agree shouldn't change).

Thank you for your reply. It appears there is a ~5% performance hit by enabling
that option.

[Bug target/112470] [11/12/13/14 regression] [AARCH64] stack-protector vulnerability fixing solution impact code size and performance

[pinskia at gcc dot gnu.org]

https://gcc.gnu.org/bugzilla/show_bug.cgi?id=112470

--- Comment #9 from Andrew Pinski <pinskia at gcc dot gnu.org> ---
Created attachment 56921
  --> https://gcc.gnu.org/bugzilla/attachment.cgi?id=56921&action=edit
Simple testcase

[Bug target/112470] [11/12/13/14 regression] [AARCH64] stack-protector vulnerability fixing solution impact code size and performance

[pinskia at gcc dot gnu.org]

https://gcc.gnu.org/bugzilla/show_bug.cgi?id=112470

--- Comment #10 from Andrew Pinski <pinskia at gcc dot gnu.org> ---
The instruction increase is 2:
        sub     sp, sp, #128
...
        stp     x29, x30, [sp, 112]

vs:
        stp     x29, x30, [sp, -128]!


and
        ldp     x29, x30, [sp, 112]
...
        add     sp, sp, 128

vs:
        ldp     x29, x30, [sp], 128

Depending on the core, the performance might be the same.

Without a full performance testcase which shows the difference, it is hard to
tell if this is an issue overall or just some one that shows up in one small
testcase.

[Bug target/112470] [11/12/13/14 regression] [AARCH64] stack-protector vulnerability fixing solution impact code size and performance

[pinskia at gcc dot gnu.org]

https://gcc.gnu.org/bugzilla/show_bug.cgi?id=112470

Andrew Pinski <pinskia at gcc dot gnu.org> changed:

           What    |Removed                     |Added
----------------------------------------------------------------------------
             Status|UNCONFIRMED                 |WAITING
     Ever confirmed|0                           |1
   Last reconfirmed|                            |2024-01-25

--- Comment #11 from Andrew Pinski <pinskia at gcc dot gnu.org> ---
.

[Bug target/112470] [11/12/13/14 regression] [AARCH64] stack-protector vulnerability fixing solution impact code size and performance

[xry111 at gcc dot gnu.org]

https://gcc.gnu.org/bugzilla/show_bug.cgi?id=112470

Xi Ruoyao <xry111 at gcc dot gnu.org> changed:

           What    |Removed                     |Added
----------------------------------------------------------------------------
                 CC|                            |xry111 at gcc dot gnu.org

--- Comment #12 from Xi Ruoyao <xry111 at gcc dot gnu.org> ---
I'm wondering why this is a P1...  To me this is making every
missed-optimization for a potential 0.5% performance increment candidates of
P1.

[Bug target/112470] [11/12/13/14 regression] [AARCH64] stack-protector vulnerability fixing solution impact code size and performance

[jakub at gcc dot gnu.org]

https://gcc.gnu.org/bugzilla/show_bug.cgi?id=112470

Jakub Jelinek <jakub at gcc dot gnu.org> changed:

           What    |Removed                     |Added
----------------------------------------------------------------------------
                 CC|                            |jakub at gcc dot gnu.org

--- Comment #13 from Jakub Jelinek <jakub at gcc dot gnu.org> ---
So, is there anything we should do about this PR, or just close it as necessary
outcome of trying to be more secure when user asked for it?

[Bug target/112470] [11/12/13/14 regression] [AARCH64] stack-protector vulnerability fixing solution impact code size and performance

[xry111 at gcc dot gnu.org]

https://gcc.gnu.org/bugzilla/show_bug.cgi?id=112470

--- Comment #14 from Xi Ruoyao <xry111 at gcc dot gnu.org> ---
(In reply to Jakub Jelinek from comment #13)
> So, is there anything we should do about this PR, or just close it as
> necessary outcome of trying to be more secure when user asked for it?

At least it shouldn't be a "P1 critical."

[Bug target/112470] [11/12/13/14 regression] [AARCH64] stack-protector vulnerability fixing solution impact code size and performance

[rguenth at gcc dot gnu.org]

https://gcc.gnu.org/bugzilla/show_bug.cgi?id=112470

Richard Biener <rguenth at gcc dot gnu.org> changed:

           What    |Removed                     |Added
----------------------------------------------------------------------------
           Priority|P1                          |P2

--- Comment #15 from Richard Biener <rguenth at gcc dot gnu.org> ---
P2 since it's a minor missed-optimization and possibly an unwanted optimization
even.  "importance" is priority of the reporter, so I'm leaving that critical.