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

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

On Mon, Nov 20, 2023 at 1:40 PM Alexandre Oliva <oliva@adacore.com> wrote:
>
> On Oct 26, 2023, Alexandre Oliva <oliva@adacore.com> wrote:
>
> >> This is a refreshed and improved version of the version posted back in
> >> June.  https://gcc.gnu.org/pipermail/gcc-patches/2023-June/621936.html
>
> > Ping? https://gcc.gnu.org/pipermail/gcc-patches/2023-October/633675.html
> > I'm combining the gcc/ipa-strub.cc bits from
> > https://gcc.gnu.org/pipermail/gcc-patches/2023-October/633526.html
>
> Ping?
> Retested on x86_64-linux-gnu, with and without -fstrub=all.

@@ -898,7 +899,24 @@ decl_attributes (tree *node, tree attributes, int flags,
       TYPE_NAME (tt) = *node;
     }

-  *anode = cur_and_last_decl[0];
+  if (*anode != cur_and_last_decl[0])
+    {
+      /* Even if !spec->function_type_required, allow the attribute
+ handler to request the attribute to be applied to the function
+ type, rather than to the function pointer type, by setting
+ cur_and_last_decl[0] to the function type.  */
+      if (!fn_ptr_tmp
+  && POINTER_TYPE_P (*anode)
+  && TREE_TYPE (*anode) == cur_and_last_decl[0]
+  && FUNC_OR_METHOD_TYPE_P (TREE_TYPE (*anode)))
+ {
+  fn_ptr_tmp = TREE_TYPE (*anode);
+  fn_ptr_quals = TYPE_QUALS (*anode);
+  anode = &fn_ptr_tmp;
+ }
+      *anode = cur_and_last_decl[0];
+    }
+

what is this a workaround for?  Isn't there a suitable parsing position
for placing the attribute?

+#ifndef STACK_GROWS_DOWNWARD
+# define STACK_TOPS GT
+#else
+# define STACK_TOPS LT
+#endif

according to docs this is defined to 0 or 1 so the above looks wrong
(it's always defined).

+  if (optimize < 2 || optimize_size || flag_no_inline)
+    return NULL_RTX;

I'm wondering about these checks in the expansions of the builtins,
I think this is about inline expanding or emitting a libcall, right?
I wonder if you should use optimize_function_for_speed (cfun) instead?
Usually -fno-inline shouldn't affect such calls, but -fno-builtin-FOO would.
I have no strong opinion here though.

The new builtins seem undocumented - usually those are documented
within extend.texi - I guess placing __builtin___strub_enter calls in
the code manually will break in interesting ways - if that's not supposed
to happen the trick is to embed a space in the name of the built-in.
__builtin_stack_address looks like something users will pick up though
(and thus should be documented)?

-symtab_node::reset (void)
+symtab_node::reset (bool preserve_comdat_group)

not sure what for, I'll leave Honza to comment.

+/* Create a distinct copy of the type of NODE's function, and change
+   the fntype of all calls to it with the same main type to the new
+   type.  */
+
+static void
+distinctify_node_type (cgraph_node *node)
+{
+  tree old_type = TREE_TYPE (node->decl);
+  tree new_type = build_distinct_type_copy (old_type);
+  tree new_ptr_type = NULL_TREE;
+
+  /* Remap any calls to node->decl that use old_type, or a variant
+     thereof, to new_type as well.  We don't look for aliases, their
+     declarations will have their types changed independently, and
+     we'll adjust their fntypes then.  */
+  for (cgraph_edge *e = node->callers; e; e = e->next_caller)
+    {
+      if (!e->call_stmt)
+ continue;
+      tree fnaddr = gimple_call_fn (e->call_stmt);
+      gcc_checking_assert (TREE_CODE (fnaddr) == ADDR_EXPR
+   && TREE_OPERAND (fnaddr, 0) == node->decl);
+      if (strub_call_fntype_override_p (e->call_stmt))
+ continue;
+      if (!new_ptr_type)
+ new_ptr_type = build_pointer_type (new_type);
+      TREE_TYPE (fnaddr) = new_ptr_type;
+      gimple_call_set_fntype (e->call_stmt, new_type);
+    }
+
+  TREE_TYPE (node->decl) = new_type;

it does feel like there's IPA mechanisms to deal with what you are trying to do
here (or in the caller(s)).


+unsigned int
+pass_ipa_strub_mode::execute (function *)
+{
+  last_cgraph_order = 0;
+  ipa_strub_set_mode_for_new_functions ();
+
+  /* Verify before any inlining or other transformations.  */
+  verify_strub ();

if  (flag_checking) verify_strub ();

please.  I guess we talked about this last year - what's the reason to have both
an IPA pass and a simple IPA pass?  IIRC the simple IPA pass is a simple
one because it wants to see inlined bodies and "fixes" those up?  Some toplevel
comments explaining both passes in the ipa-strub.cc pass would be nice to
have.  I guess I also asked before - did you try it with -flto?

+    /* Decide which of the wrapped function's parms we want to turn into
+       references to the argument passed to the wrapper.  In general,
we want to
+       copy small arguments, and avoid copying large ones.
Variable-sized array
+       lengths given by other arguments, as in 20020210-1.c, would lead to
+       problems if passed by value, after resetting the original function and
+       dropping the length computation; passing them by reference works.
+       DECL_BY_REFERENCE is *not* a substitute for this: it involves copying
+       anyway, but performed at the caller.  */
+    indirect_parms_t indirect_nparms (3, false);
...

IMHO it's a bad idea, at least initially, to mess with the ABI in a heuristical
way this way.  I see you need all sorts of "fixup", including re-gimplification
of stmts, etc., double-ugh.  Ideally IPA-SRA can already do parts of those
transforms which would mean ipa-param-manipulation has generic code
to encode them?  I realize you can't tail-call, but I wonder if some clever
machine-specific tricks would work, I guess it would need the wrapper
to be target generated of course.

As it's only for optimization, how much of the code would go away when
you avoid changing the parameters of the wrapped function?  Could
one easily disable the optimization via a --param maybe?

There's still much commented code and FIXMEs in, a lot of cgraph
related code is used which I can't review very well.

I fear that -fstrub is going to be actually used by people - are you up
to the task fixing things?

+void ATTRIBUTE_STRUB_CALLABLE
+__strub_enter (void **watermark)
+{
+  *watermark = __builtin_frame_address (0);

in your experience, do these (or the inline version) work reliably
when optimizations like omitting the frame pointer or shrink-wrapping
happen from a security point of view?

You are adding symbols to libgcc but I don't see you amending
libgcc/libgcc-std.ver.in?

Neither the documentation in extend.texi nor the one in invoke.texi
mentions the target audience of -fstrub - I suppose it is to ensure
secrets are erased from the stack in case they are stored into locals
or spilled and thus it complements -fzero-call-used-regs.  The patches
mention the red zone, documenting the guarantees and caveats
would be nice to have.  I think the documentation belongs to the
attribute documentation.

I hope Honza can have a quick look over the IPA passes.  As said,
eliding the parmeter optimization from this patch, possibly
implementing missing pieces in ipa-param-manipulation might be
a better way than taking what is percieved as a huge ugly part
of the patch.

Thanks,
Richard.

> >> This patch adds the strub attribute for function and variable types,
> >> command-line options, passes and adjustments to implement it,
> >> documentation, and tests.
>
> >> Stack scrubbing is implemented in a machine-independent way: functions
> ...
>
> --
> 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