Re: [PATCH v4] Introduce strub: machine-independent stack scrubbing

[Richard Biener <richard.guenther@gmail.com>]

On Wed, Nov 29, 2023 at 9:53 AM Alexandre Oliva <oliva@adacore.com> wrote:
>
> On Nov 23, 2023, Richard Biener <richard.guenther@gmail.com> wrote:
>
> > Conceptually it shouldn't be much different from what IPA-SRA does
> > which is cloning a function but with different arguments, the function
> > signature transform described in terms of ipa-param-manipulation bits.
> > I've talked with Martin and at least there's currently no
> > by-value-to-by-reference
> > "transform", but IPA-SRA can pass two registers instead of one aggregate
> > for example.  There's IPA_PARAM_OP_NEW already to add a new param.
> > In principle the whole function rewriting (apart of recovering
> > from inlining) should be doable within this framework.
>
> I agree, but my attempts to do so have been unfruitful, and hit various
> very obscure issues that made it unworkable.  Maybe someone smarter than
> I am, or with more time than I had, can make the conversion.  The uses
> of IPA_PARAM_OP_NEW for the cloning are there, and the more conservative
> choices I made got it to work reliably, unlike the more adventurous
> alternatives I tried.  One of the obscure issues I recall hitting was
> this one.  I can probably still locate the early strub patch that hit
> the described issue back then, if there's interest.
> https://gcc.gnu.org/pipermail/gcc-patches/2021-July/575044.html
>
> >> The
> >> wrapper already needs a custom ABI, because of the added watermark
> >> pointer, and va_list and whatnot when needed, so a little further
> >> customization to avoid a quite significant overhead seemed desirable.
>
> > I understood the purpose of this, but I also saw it's only ever needed for
> > non-strict operation
>
> Erhm...  I suppose you're not talking about -fstrub=strict, because I
> can't see how this relates with the by-reference argument passing from
> wrapper to wrapped in internal strubbing.
>
> > and I think that if you care about security you'd never
> > want to operate in a way that you don't absolutely know the function
> > you call isn't going to scrub your secrets ...
>
> The ABI between wrapper and wrapped function splitting in internal strub
> doesn't change this.
>
> > But yes, I also wondered why you even run into re-gimplification issues.
>
> Because &arg_#(D)[n_#] is good gimple, but &(*byref_arg_#(D))[n_#] isn't.

'arg_#(D)' looks like a SSA name, and no, taking the address doesn't work,
so I assume it was &MEM[arg_(D)][n_#] which is indeed OK.  But you
shouldn't need to change a pointer argument to be passed by reference,
do you?  As said, you want to restrict by-reference passing to arguments
that are !is_gimple_reg_type ().  Everywhere where a plain PARM_DECL
was valid a *ptr indirection is as well.

> Maybe instead of going through regimplify, we could explicitly create an
> SSA name for the indirection load, so that the purpose is made more
> explicit.
>
> > replacing the PARM_DECL there with a MEM_REF (new-PARM_DECL) should
> > work without re-gimplification.
>
> FWIW, I thought so as well ;-)
>
> > I didn't remember seeing that you do sth like wrapping
> > each "strubbed call" inside a try { call(); } finally { do-strub } to
> > ensure this.
> > Guess it was well hidden in the large patch ;)
>
> Indeed, gsi_insert_finally_seq_after_call is where this is taken care of.
>
> >> > As it's only for optimization, how much of the code would go away when
> >> > you avoid changing the parameters of the wrapped function?
> >>
> >> We can't avoid changing them entirely, and IIRC some of the
> >> infrastructure for regimplification was also used for va_list and
> >> apply_args handling, so it wouldn't save all that much.
>
> > Ah, var-args ... indeed for simple forwarding it shouldn't be too bad.
> > I wonder if this were a way to remove the restriction on function
> > splitting of var-args functions - there's a recent bugreport about that
> > (before any va_arg () has been called, of course).
>
> If running va_start unconditionally in varargs functions is generally
> acceptable, then the method I've used for wrapping varargs functions
> should work generally, yeah.  The trick was to turn:
>
> foo (...)
> {
>   va_list ap;
>
>   ...
>   va_start (&ap, <ignored>);
>   ...
> }
>
> into
>
> wrapped_foo (va_list &wrap)
> {
>   va_list ap;
>
>   ...
>   va_copy (ap, wrap);
>   ...
> }
>
> foo (...)
> {
>   va_list wrap;
>
>   va_start (wrap, <also ignored>);
>   wrapped_foo (wrap);
>   va_end (wrap);
> }

Ah, nice trick indeed.  I think that should work, it might not
be optimal (I don't think we will optimize the va_copy).

>
> Here's the opening comment I added to ipa-strub.cc:
>
> /* This file introduces two passes that, together, implement
>    machine-independent stack scrubbing, strub for short.  It arranges
>    for stack frames that have strub enabled to be zeroed-out after
>    relinquishing control to a caller, whether by returning or by
>    propagating an exception.  This admittedly unusual design decision
>    was driven by exception support (one needs a stack frame to be
>    active to propagate exceptions out of it), and it enabled an
>    implementation that is entirely machine-independent (no custom
>    epilogue code is required).
>
>    Strub modes can be selected for stack frames by attaching attribute
>    strub to functions or to variables (to their types, actually).
>    Different strub modes, with different implementation details, are
>    available, and they can be selected by an argument to the strub
>    attribute.  When enabled by strub-enabled variables, whether by
>    accessing (as in reading from) statically-allocated ones, or by
>    introducing (as in declaring) automatically-allocated ones, a
>    suitable mode is selected automatically.
>
>    At-calls mode modifies the interface of a function, adding a stack
>    watermark argument, that callers use to clean up the stack frame of
>    the called function.  Because of the interface change, it can only
>    be used when explicitly selected, or when a function is internal to
>    a translation unit.  Strub-at-calls function types are distinct
>    from their original types (they're not modified in-place), and they
>    are not interchangeable with other function types.
>
>    Internal mode, in turn, does not modify the type or the interface
>    of a function.  It is currently implemented by turning the function
>    into a wrapper, moving the function body to a separate wrapped
>    function, and scrubbing the wrapped body's stack in the wrapper.
>    Internal-strub function types are mostly interface-compatible with
>    other strub modes, namely callable (from strub functions, though
>    not strub-enabled) and disabled (not callable from strub
>    functions).
>
>    Always_inline functions can be strub functions, but they can only
>    be called from other strub functions, because strub functions must
>    never be inlined into non-strub functions.  Internal and at-calls
>    modes are indistinguishable when it comes to always_inline
>    functions: they will necessarily be inlined into another strub
>    function, and will thus be integrated into the caller's stack
>    frame, whatever the mode.  (Contrast with non-always_inline strub
>    functions: an at-calls function can be called from other strub
>    functions, ensuring no discontinuity in stack erasing, whereas an
>    internal-strub function can only be called from other strub
>    functions if it happens to be inlined, or if -fstrub=relaxed mode
>    is in effect (that's the default).  In -fstrub=strict mode,
>    internal-strub functions are not callable from strub functions,
>    because the wrapper itself is not strubbed.
>
>    The implementation involves two simple-IPA passes.  The earliest
>    one, strub-mode, assigns strub modes to functions.  It needs to run
>    before any inlining, so that we can prevent inlining of strub
>    functions into non-strub functions.  It notes explicit strub mode
>    requests, enables strub in response to strub variables and testing
>    options, and flags unsatisfiable requests.
>
>    Three possibilities of unsatisfiable requests come to mind: (a)
>    when a strub mode is explicitly selected, but the function uses
>    features that make it ineligible for that mode (e.g. at-calls rules
>    out calling __builtin_apply_args, because of the interface changes,
>    and internal mode rules out noclone or otherwise non-versionable
>    functions, non-default varargs, non-local or forced labels, and
>    functions with far too many arguments); (b) when some strub mode
>    must be enabled because of a strub variable, but the function is
>    not eligible or not viable for any mode; and (c) when
>    -fstrub=strict is enabled, and calls are found in strub functions
>    to functions that are not callable from strub contexts.
>    compute_strub_mode implements (a) and (b), and verify_strub
>    implements (c).
>
>    The second IPA pass modifies interfaces of at-calls-strub functions
>    and types, introduces strub calls in and around them. and splits
>    internal-strub functions.  It is placed after early inlining, so
>    that even internal-strub functions get a chance of being inlined
>    into other strub functions, but before non-early inlining, so that
>    internal-strub wrapper functions still get a chance of inlining
>    after splitting.
>
>    Wrappers avoid duplicating the copying of large arguments again by
>    passing them by reference to the wrapped bodies.  This involves
>    occasional SSA rewriting of address computations, because of the
>    additional indirection.  Besides these changes, and the
>    introduction of the stack watermark parameter, wrappers and wrapped
>    functions cooperate to handle variable argument lists (performing
>    va_start in the wrapper, passing the list as an argument, and
>    replacing va_start calls in the wrapped body with va_copy), and
>    __builtin_apply_args (also called in the wrapper and passed to the
>    wrapped body as an argument).
>
>    Strub bodies (both internal-mode wrapped bodies, and at-calls
>    functions) always start by adjusting the watermark parameter, by
>    calling __builtin___strub_update.  The compiler inserts them in the
>    main strub pass.  Allocations of additional stack space for the
>    frame (__builtin_alloca) are also followed by watermark updates.
>    Stack space temporarily allocated to pass arguments to other
>    functions, released right after the call, is not regarded as part
>    of the frame.  Around calls to them, i.e., in internal-mode
>    wrappers and at-calls callers (even calls through pointers), calls
>    to __builtin___strub_enter and __builtin___strub_leave are
>    inserted, the latter as a __finally block, so that it runs at
>    regular and exceptional exit paths.  strub_enter only initializes
>    the stack watermark, and strub_leave is where the scrubbing takes
>    place, overwriting with zeros the stack space from the top of the
>    stack to the watermark.
>
>    These calls can be optimized in various cases.  In
>    pass_ipa_strub::adjust_at_calls_call, for example, we enable
>    tail-calling and other optimized calls from one strub body to
>    another by passing on the watermark parameter.  The builtins
>    themselves may undergo inline substitution during expansion,
>    dependign on optimization levels.  This involves dealing with stack
>    red zones (when the builtins are called out-of-line, the red zone
>    cannot be used) and other ugly details related with inlining strub
>    bodies into other strub bodies (see expand_builtin_strub_update).
>    expand_builtin_strub_leave may even perform partial inline
>    substitution.  */

Thanks.

> The type attribute description now starts like this:
>
> @cindex @code{strub} type attribute
> @item strub
> This attribute defines stack-scrubbing properties of functions and
> variables, so that functions that access sensitive data can have their
> stack frames zeroed-out upon returning or propagating exceptions.  This
> may be enabled explicitly, by selecting certain @code{strub} modes for
> specific functions, or implicitly, by means of @code{strub} variables.
>
> And I've fixed the symbol versioning for strub and hardcfr functions.
>
> I've just pushed the base strub patch and the recent incremental changes
> (and some commits used for testing) to refs/users/aoliva/heads/strub.
>
>
> Here are changes.html entries for this and for the other newly-added
> features:
>
> new AdaCore-contributed hardening features in gcc 13 and 14
>
> Mention hardening of conditionals (added in gcc 13), control flow
> redundancy, hardened booleans, and stack scrubbing.
>
> Also cover forced inlining of string operations while at that.
> ---
>  htdocs/gcc-13/changes.html |    6 ++++++
>  htdocs/gcc-14/changes.html |   29 +++++++++++++++++++++++++++++
>  2 files changed, 35 insertions(+)
>
> diff --git a/htdocs/gcc-13/changes.html b/htdocs/gcc-13/changes.html
> index 8ef3d63952daf..c1dea18caf59b 100644
> --- a/htdocs/gcc-13/changes.html
> +++ b/htdocs/gcc-13/changes.html
> @@ -168,6 +168,12 @@ You may also want to check out our
>      been added, see also
>      <a href="https://gcc.gnu.org/onlinedocs/gcc/Freestanding-Environments.html">Profiling and Test Coverage in Freestanding Environments</a>.
>    </li>
> +  <li>
> +    New options <code>-fharden-compares</code>
> +    and <code>-fharden-conditional-branches</code> to verify compares
> +    and conditional branches, to detect some power-deprivation
> +    hardware attacks, using reversed conditions.
> +  </li>
>  </ul>
>
>
> diff --git a/htdocs/gcc-14/changes.html b/htdocs/gcc-14/changes.html
> index 2088ee91a34e1..cb92f8d8095c3 100644
> --- a/htdocs/gcc-14/changes.html
> +++ b/htdocs/gcc-14/changes.html
> @@ -109,6 +109,35 @@ a work-in-progress.</p>
>      of hardening flags.  The options it enables can be displayed using the
>      <code>--help=hardened</code> option.
>    </li>
> +  <li>
> +    New option <code>-fharden-control-flow-redundancy</code>, to
> +    verify, at the end of functions, that the visited basic blocks
> +    correspond to a legitimate execution path, so as to detect and
> +    prevent attacks that transfer control into the middle of
> +    functions.
> +  </li>
> +  <li>
> +    New type attribute <code>hardbool</code>, for C and Ada.  Hardened
> +    booleans take user-specified representations for <code>true</code>
> +    and <code>false</code>, presumably with higher hamming distance
> +    than standard booleans, and get verified at every use, detecting
> +    memory corruption and some malicious attacks.
> +  </li>
> +  <li>
> +    New type attribute <code>strub</code> to control stack scrubbing
> +    properties of functions and variables.  The stack frame used by
> +    functions marked with the attribute gets zeroed-out upon returning
> +    or exception escaping.  Scalar variables marked with the attribute
> +    cause functions contaning or accessing them to get stack scrubbing
> +    enabled implicitly.
> +  </li>
> +  <li>
> +    New option <code>-finline-stringops</code>, to force inline
> +    expansion of <code>memcmp</code>, <code>memcpy</code>,
> +    <code>memmove</code> and <code>memset</code>, even when that is
> +    not an optimization, to avoid relying on library
> +    implementations.
> +  </li>
>  </ul>
>  <!-- .................................................................. -->
>  <h2 id="languages">New Languages and Language specific improvements</h2>

LGTM.

Can you check on the pass-by-reference thing again please?

Let's see if Honza or Martin have any comments on the IPA bits, I just
mentioned what I think should be doable ...

Thanks,
Richard.

>
> --
> Alexandre Oliva, happy hacker            https://FSFLA.org/blogs/lxo/
>    Free Software Activist                   GNU Toolchain Engineer
> More tolerance and less prejudice are key for inclusion and diversity
> Excluding neuro-others for not behaving ""normal"" is *not* inclusive