From mboxrd@z Thu Jan 1 00:00:00 1970 Return-Path: Received: from mail-lf1-x134.google.com (mail-lf1-x134.google.com [IPv6:2a00:1450:4864:20::134]) by sourceware.org (Postfix) with ESMTPS id 663CE3858D39 for ; Wed, 22 Nov 2023 14:18:06 +0000 (GMT) DMARC-Filter: OpenDMARC Filter v1.4.2 sourceware.org 663CE3858D39 Authentication-Results: sourceware.org; dmarc=pass (p=none dis=none) header.from=gmail.com Authentication-Results: sourceware.org; spf=pass smtp.mailfrom=gmail.com ARC-Filter: OpenARC Filter v1.0.0 sourceware.org 663CE3858D39 Authentication-Results: server2.sourceware.org; arc=none smtp.remote-ip=2a00:1450:4864:20::134 ARC-Seal: i=1; a=rsa-sha256; d=sourceware.org; s=key; t=1700662688; cv=none; b=fanGe9NageYnTp/nKo06t46uAf6gRoCma6g6xy3/FZ7hmKWOOmWe8Pxr54oZMtPkcEicmKR8SxUaUjF0GqMoFUTHChl3h3zomWO99v6F8KP+SoflAlDFkECNsmQGqYm6yvdCP2VxbGNR6sr1NqTH3ey+owpQr4JKVJewJzOyfSk= ARC-Message-Signature: i=1; a=rsa-sha256; d=sourceware.org; s=key; t=1700662688; c=relaxed/simple; bh=wwIZ8i03yC43xBM/EhukIpiCeHthwmIF+OW4hRUqusY=; h=DKIM-Signature:MIME-Version:From:Date:Message-ID:Subject:To; b=hPrf+H+5tBIUjmNhalZBC6Mx5Q/0ZldKOl2RDKLo0Toa6M3r6VkXzXBRuJyjc0XsN9/laQb0fP5BFxEQBayf7ugyzhqoyQEODSXOv1/2HNKzGqSc95v4jJ3li5J2KkGYRVo7DKmbkVPuXgB7zAmGi/gp7kIuMwOM/x2SB2f1vlg= ARC-Authentication-Results: i=1; server2.sourceware.org Received: by mail-lf1-x134.google.com with SMTP id 2adb3069b0e04-507f1c29f25so9371096e87.1 for ; Wed, 22 Nov 2023 06:18:06 -0800 (PST) DKIM-Signature: v=1; a=rsa-sha256; c=relaxed/relaxed; d=gmail.com; s=20230601; t=1700662685; x=1701267485; darn=gcc.gnu.org; h=content-transfer-encoding:cc:to:subject:message-id:date:from :in-reply-to:references:mime-version:from:to:cc:subject:date :message-id:reply-to; bh=PwiW6gr3CdjR4EWL5fIZdg0r5wyVHJ5OSyXuZi69zSg=; b=UW/m/bdnOlkKPB4ZDw3lqoOCqqnl4tgO28N5Xc4lLbxe6i4+S022R8//ZAiWU6g+q0 LUwU940c9hMpnbZW1BPRG68jtKOy/P82xcp7pD256H2df0xg7RpcmYUbMjU/Scevf1IL ufIklXpL5SF31SCzlFRELWtOqEkqnz6zMFSeWJkKNzBT8ZHX+jGf536+VljPNuWaKVI8 nUm3Lc4eLXkFhcCNHXtDH2O4V7mPh5yvcNjyoac83CpYvgj/m04skNG1oi8eXSt5n3mv Bud4NgsgKiyov4rpbIA8ZFpyJISDdAT/J4zVIJvHQMU5iMmQGFklCC/sawbtxA6hCN8A rLGw== X-Google-DKIM-Signature: v=1; a=rsa-sha256; c=relaxed/relaxed; d=1e100.net; s=20230601; t=1700662685; x=1701267485; h=content-transfer-encoding:cc:to:subject:message-id:date:from :in-reply-to:references:mime-version:x-gm-message-state:from:to:cc :subject:date:message-id:reply-to; bh=PwiW6gr3CdjR4EWL5fIZdg0r5wyVHJ5OSyXuZi69zSg=; b=ijE8cnhS7pIFsJRIaWiQ4Wyx0rygbnoecFVtSEMDVumlYcAkfdFwoZ7ngM8qP0KfiF 1duijQZB9hxeCh7OyG24f96aNgY5ZMoSwZtMUu1F+bBjzdGo5AszBHKMSSZqxoCgUiQ0 THBqjhErkWrv6+nQSDL5UhZkxrfX49w7uOH3uWOL+2Z1K2yMbc/r60XpVCYTc60+PgPt 5huT0qhBBmRfg+J+U/Nw2Ocg5FavQWmyxKeTgV/qQbTgMA4noi/PA1uIgDhi2p9o0UiU zWV1oYRkZhjTvM8EJ7xmbd3q6oIJV6yIL/CBJuolszXMvN0AQYleG0q06MDVOA6yPY5F D5Mw== X-Gm-Message-State: AOJu0Yx0n5iZKctrtR8/gjrtsjpGisitAc0BfoRJ4C/8ypI0t77Kj2TF 5jxO42ePI5cs4A4NWd4fTJpqTiW2mbvraf8jIV8= X-Google-Smtp-Source: AGHT+IHbY1Hl/JocQsKbEqr95/6MqkzqVsP9Ak7K/w2/K7B1kFjWW8Gd82QSMxtun63ym52lLPvjBZnKDOlS2wOS5kY= X-Received: by 2002:ac2:5975:0:b0:50a:71e1:e1ce with SMTP id h21-20020ac25975000000b0050a71e1e1cemr1657879lfp.51.1700662684551; Wed, 22 Nov 2023 06:18:04 -0800 (PST) MIME-Version: 1.0 References: In-Reply-To: From: Richard Biener Date: Wed, 22 Nov 2023 15:14:23 +0100 Message-ID: Subject: Re: [PATCH v4] Introduce strub: machine-independent stack scrubbing To: Alexandre Oliva Cc: gcc-patches@gcc.gnu.org, Jeremy Bennett , Craig Blackmore , Graham Markall , Martin Jambor , Jan Hubicka , Jim Wilson , Jeff Law , Jakub Jelinek Content-Type: text/plain; charset="UTF-8" Content-Transfer-Encoding: quoted-printable X-Spam-Status: No, score=-1.5 required=5.0 tests=BAYES_00,DKIM_SIGNED,DKIM_VALID,DKIM_VALID_AU,DKIM_VALID_EF,FREEMAIL_FROM,KAM_SHORT,RCVD_IN_DNSWL_NONE,SPF_HELO_NONE,SPF_PASS,TXREP,T_SCC_BODY_TEXT_LINE,WEIRD_QUOTING autolearn=ham autolearn_force=no version=3.4.6 X-Spam-Checker-Version: SpamAssassin 3.4.6 (2021-04-09) on server2.sourceware.org List-Id: On Mon, Nov 20, 2023 at 1:40=E2=80=AFPM Alexandre Oliva = wrote: > > On Oct 26, 2023, Alexandre Oliva 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.htm= l > > 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=3Dall. @@ -898,7 +899,24 @@ decl_attributes (tree *node, tree attributes, int flag= s, TYPE_NAME (tt) =3D *node; } - *anode =3D cur_and_last_decl[0]; + if (*anode !=3D 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) =3D=3D cur_and_last_decl[0] + && FUNC_OR_METHOD_TYPE_P (TREE_TYPE (*anode))) + { + fn_ptr_tmp =3D TREE_TYPE (*anode); + fn_ptr_quals =3D TYPE_QUALS (*anode); + anode =3D &fn_ptr_tmp; + } + *anode =3D 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 =3D TREE_TYPE (node->decl); + tree new_type =3D build_distinct_type_copy (old_type); + tree new_ptr_type =3D 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 =3D node->callers; e; e =3D e->next_caller) + { + if (!e->call_stmt) + continue; + tree fnaddr =3D gimple_call_fn (e->call_stmt); + gcc_checking_assert (TREE_CODE (fnaddr) =3D=3D ADDR_EXPR + && TREE_OPERAND (fnaddr, 0) =3D=3D node->decl); + if (strub_call_fntype_override_p (e->call_stmt)) + continue; + if (!new_ptr_type) + new_ptr_type =3D build_pointer_type (new_type); + TREE_TYPE (fnaddr) =3D new_ptr_type; + gimple_call_set_fntype (e->call_stmt, new_type); + } + + TREE_TYPE (node->decl) =3D new_type; it does feel like there's IPA mechanisms to deal with what you are trying t= o do here (or in the caller(s)). +unsigned int +pass_ipa_strub_mode::execute (function *) +{ + last_cgraph_order =3D 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 topl= evel 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 copyi= ng + 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 heurist= ical way this way. I see you need all sorts of "fixup", including re-gimplifica= tion 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 =3D __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