Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

cmake: Disable Control-Flow check instrumentation #649

Open
wants to merge 1 commit into
base: master
Choose a base branch
from
Open

Conversation

chfast
Copy link
Member

@chfast chfast commented May 15, 2023

Disable Control-Flow protection (CET) for the Baseline interpreter. This is controlled by -fcf-protection GCC/Clang compiler flag and may be enabled by default depending on the OS configuration.

This is disabled because it slightly affects performance and code size. If the CF protection is desired it should be enabled uniformly in all compilers which support it.

Somehow, applying [[nocf_check]] attribute has no desired effect.

See GCC documentation:
https://gcc.gnu.org/onlinedocs/gcc/Instrumentation-Options.html#index-fcf-protection

Disable Control-Flow protection (CET) for the Baseline interpreter.
This is controlled by -fcf-protection GCC/Clang compiler flag and may
be enabled by default depending on the OS configuration.

This is disabled because it slightly affects performance and code size.
If the CF protection is desired it should be enabled uniformly in all
compilers which support it.

Somehow, applying [[nocf_check]] attribute has no desired effect.

See GCC documentation:
https://gcc.gnu.org/onlinedocs/gcc/Instrumentation-Options.html#index-fcf-protection
@chfast
Copy link
Member Author

chfast commented May 15, 2023

GCC 13, Skylake

baseline/execute/main/blake2b_huff/empty_mean                                -0.0072         -0.0074            11            11            11            11
baseline/execute/main/blake2b_huff/8415nulls_mean                            -0.0107         -0.0108           697           690           697           690
baseline/execute/main/blake2b_shifts/8415nulls_mean                          -0.0069         -0.0070          6718          6671          6717          6670
baseline/execute/main/sha1_divs/empty_mean                                   -0.0188         -0.0189            50            49            50            49
baseline/execute/main/sha1_divs/5311_mean                                    -0.0160         -0.0161          4013          3949          4012          3948
baseline/execute/main/sha1_shifts/empty_mean                                 -0.0316         -0.0317            24            23            24            23
baseline/execute/main/sha1_shifts/5311_mean                                  -0.0358         -0.0359          1973          1902          1972          1901
baseline/execute/main/snailtracer/benchmark_mean                             -0.0247         -0.0248         39929         38944         39923         38935
baseline/execute/main/structarray_alloc/nfts_rank_mean                       -0.0140         -0.0141           445           438           445           438
baseline/execute/main/swap_math/spent_mean                                   -0.0432         -0.0433             2             2             2             2
baseline/execute/main/swap_math/received_mean                                -0.0176         -0.0176             3             2             3             2
baseline/execute/main/swap_math/insufficient_liquidity_mean                  -0.0045         -0.0046             2             1             2             1
baseline/execute/main/weierstrudel/1_mean                                    -0.0349         -0.0350           205           198           205           198
baseline/execute/main/weierstrudel/15_mean                                   -0.0172         -0.0172          1996          1961          1995          1961
OVERALL_GEOMEAN                                                              -0.0203         -0.0204             0             0             0             0

@codecov
Copy link

codecov bot commented May 15, 2023

Codecov Report

All modified and coverable lines are covered by tests ✅

Comparison is base (d63fa8b) 97.33% compared to head (a6ebe60) 97.33%.
Report is 238 commits behind head on master.

Additional details and impacted files
@@           Coverage Diff           @@
##           master     #649   +/-   ##
=======================================
  Coverage   97.33%   97.33%           
=======================================
  Files          80       80           
  Lines        7739     7739           
=======================================
  Hits         7533     7533           
  Misses        206      206           
Flag Coverage Δ
blockchaintests 64.17% <ø> (ø)
statetests 74.55% <ø> (ø)
unittests 94.83% <ø> (ø)

Flags with carried forward coverage won't be shown. Click here to find out more.

@chfast
Copy link
Member Author

chfast commented May 15, 2023

GCC 12, Haswell

baseline/execute/main/blake2b_huff/empty_mean                                +0.0023         +0.0023             9             9             9             9
baseline/execute/main/blake2b_huff/8415nulls_mean                            -0.0030         -0.0030           581           579           581           579
baseline/execute/main/blake2b_shifts/8415nulls_mean                          -0.0076         -0.0076          5531          5489          5531          5489
baseline/execute/main/sha1_divs/empty_mean                                   -0.0188         -0.0188            41            40            41            40
baseline/execute/main/sha1_divs/5311_mean                                    -0.0162         -0.0162          3279          3226          3279          3226
baseline/execute/main/sha1_shifts/empty_mean                                 -0.0286         -0.0286            20            19            20            19
baseline/execute/main/sha1_shifts/5311_mean                                  -0.0322         -0.0322          1634          1582          1634          1582
baseline/execute/main/snailtracer/benchmark_mean                             -0.0087         -0.0087         31172         30902         31172         30902
baseline/execute/main/structarray_alloc/nfts_rank_mean                       -0.0123         -0.0123           358           353           358           353
baseline/execute/main/swap_math/spent_mean                                   -0.0281         -0.0281             2             2             2             2
baseline/execute/main/swap_math/received_mean                                -0.0316         -0.0316             2             2             2             2
baseline/execute/main/swap_math/insufficient_liquidity_mean                  +0.0066         +0.0066             1             1             1             1
baseline/execute/main/weierstrudel/1_mean                                    -0.0062         -0.0062           176           175           176           175
baseline/execute/main/weierstrudel/15_mean                                   +0.0014         +0.0014          1673          1675          1673          1675
OVERALL_GEOMEAN                                                              -0.0132         -0.0132             0             0             0             0

@axic
Copy link
Member

axic commented May 19, 2023

The savings are impressive, but the description is scary:

Enable code instrumentation of control-flow transfers to increase program security by checking that target addresses of control-flow transfer instructions (such as indirect function call, function return, indirect jump) are valid. This prevents diverting the flow of control to an unexpected target. This is intended to protect against such threats as Return-oriented Programming (ROP), and similarly call/jmp-oriented programming (COP/JOP).

The value branch tells the compiler to implement checking of validity of control-flow transfer at the point of indirect branch instructions, i.e. call/jmp instructions. The value return implements checking of validity at the point of returning from a function. The value full is an alias for specifying both branch and return. The value none turns off instrumentation.

The value check is used for the final link with link-time optimization (LTO). An error is issued if LTO object files are compiled with different -fcf-protection values. The value check is ignored at the compile time.

So ideally we could disable this, but enable is specifically at some places where risk is present?

@chfast
Copy link
Member Author

chfast commented May 19, 2023

So ideally we could disable this, but enable is specifically at some places where risk is present?

We need to consult this with some security experts.

Originally, I wanted to disable this only for baseline interpreter, but the attribute does not work. Probably worth to report a bug.

But we can also disable this per-file basis. It may have more sense to have this enabled for bytecode analysis.

The risk is that if there is a bug in EVM someone will be able to take over the process the EVM is running in and e.g. steal some information or mess up how it behave in the p2p network. This seems unlikely though. The attacker will be still able to crash the node.

Oh and the benchmarks are shaky. The improvements may come from smaller code size and code layout change.

@chfast chfast added the optimization Iproves performance without functional changes label Jan 3, 2024
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
optimization Iproves performance without functional changes
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants