lkml.org 
[lkml]   [2021]   [Sep]   [21]   [last100]   RSS Feed
Views: [wrap][no wrap]   [headers]  [forward] 
 
Messages in this thread
/
SubjectRe: [PATCH] [RFC/RFT]SCS:Add gcc plugin to support Shadow Call Stack
From
Date


On 9/21/21 5:22 AM, Ard Biesheuvel wrote:
> On Mon, 20 Sept 2021 at 20:53, Dan Li <ashimida@linux.alibaba.com> wrote:
>>
>> Hi Ard,
>>
>> Thanks for your comment.
>>
>> I pasted a copy of the config code in my last email, could you please check it again?
>>
>> On 9/20/21 3:18 PM, Ard Biesheuvel wrote:
>>> Hi Dan,
>>>
>>> On Sun, 19 Sept 2021 at 18:37, Dan Li <ashimida@linux.alibaba.com> wrote:
>>>>
>>>> The Clang-based shadow call stack protection has been integrated into the
>>>> mainline, but kernel compiled by gcc cannot enable this feature for now.
>>>>
>>>> This Patch supports gcc-based SCS protection by adding a plugin.
>>>>
>>>
>>> Thanks for working on this. I had a stab at this myself about 2 years
>>> ago and couldn't make it work.
>>>
>>>> For each function that x30 will be pushed onto the stack during execution,
>>>> this plugin:
>>>> 1) insert "str x30, [x18], #8" at the entry of the function to save x30
>>>> to current SCS
>>>> 2) insert "ldr x30, [x18, #-8]!" before the exit of this function to
>>>> restore x30
>>>>
>>>
>>> This logic seems sound to me, but it would be nice if someone more
>>> familiar with Clang's implementation could confirm that it is really
>>> this simple.
>>>
>>> Looking at your plugin, there is an issue with tail calls, and I don't
>>> think Clang simply disables those altogether as well, right?
>>
>> I am not familiar with clang's code, the logic comes from clang's description and the
>> disassembled binary code for now, so it may be different from the actual situation.
>>
>
> OK
>
>> The tail call could be handled (theoretically), and I will try to solve the issue in
>> the next version.
>>>
>>>> ifdef CONFIG_SHADOW_CALL_STACK
>>>> -CC_FLAGS_SCS := -fsanitize=shadow-call-stack
>>>> +CC_FLAGS_SCS := $(if $(CONFIG_CC_IS_CLANG),-fsanitize=shadow-call-stack,)
>>>
>>> This variable should contain whatever needs to be added to the
>>> compiler comamand line
>> In the new code, an 'enable' option is added here to enable the plugin
>>>> KBUILD_CFLAGS += $(CC_FLAGS_SCS)
>>>> export CC_FLAGS_SCS
>>>> endif
>>>> diff --git a/arch/Kconfig b/arch/Kconfig
>>>> index 98db634..81ff127 100644
>>>> --- a/arch/Kconfig
>>>> +++ b/arch/Kconfig
>>>> @@ -594,7 +594,7 @@ config ARCH_SUPPORTS_SHADOW_CALL_STACK
>>>>
>>>> config SHADOW_CALL_STACK
>>>> bool "Clang Shadow Call Stack"
>>>> - depends on CC_IS_CLANG && ARCH_SUPPORTS_SHADOW_CALL_STACK
>>>> + depends on (CC_IS_CLANG && ARCH_SUPPORTS_SHADOW_CALL_STACK) || GCC_PLUGIN_SHADOW_CALL_STACK
>>>
>>> This logic needs to be defined in such a way that a builtin
>>> implementation provided by GCC will take precedence once it becomes
>>> available.
>>>
>> In new code, if gcc supports SCS in the future, the plugin will be closed due to
>> CC_HAVE_SHADOW_CALL_STACK is true.
>>>> depends on DYNAMIC_FTRACE_WITH_REGS || !FUNCTION_GRAPH_TRACER
>>>> help
>>>> This option enables Clang's Shadow Call Stack, which uses a
>>>> diff --git a/scripts/gcc-plugins/Kconfig b/scripts/gcc-plugins/Kconfig
>>>> index ab9eb4c..2534195e 100644
>>>> --- a/scripts/gcc-plugins/Kconfig
>>>> +++ b/scripts/gcc-plugins/Kconfig
>>>> @@ -19,6 +19,14 @@ menuconfig GCC_PLUGINS
>>>>
>>>> if GCC_PLUGINS
>>>>
>>>> +config GCC_PLUGIN_SHADOW_CALL_STACK
>>>> + bool "GCC Shadow Call Stack plugin"
>>>> + select SHADOW_CALL_STACK
>>>
>>> You shouldn't 'select' something like this if the symbol has its own
>>> dependencies which may be unsatisfied, as this causes a Kconfig
>>> warning. Also, en/disabling shadow call stacks for the architecture
>>> should be done from the arch's 'kernel features' menu, it shouldn't be
>>> buried in the GCC plugins menu.
>> I removed 'select' in the new version.
>> SCS's enable is changed to rely on CONFIG_SHADOW_CALL_STACK in arch/kernel,
>> the GCC_PLUGIN_SHADOW_CALL_STACK config is just to add a usable platform to it.
>>>> + help
>>>> + This plugin is used to support the kernel CONFIG_SHADOW_CALL_STACK
>>>> + compiled by gcc. Its principle is basically the same as that of CLANG.
>>>> + For more information, please refer to "config SHADOW_CALL_STACK"
>>>> +
>>>> +__visible int plugin_is_GPL_compatible;
>>>> +
>>>> +static struct plugin_info arm64_scs_plugin_info = {
>>>> + .version = "20210926vanilla",
>>>
>>> I will respond to this obvious invitation at bikeshedding by saying
>>> that 'salted caramel' is clearly the superior flavor of ice cream.
>> I'm sorry, as a non-native English speaker, I think I might not understand
>> what you mean here. My intention is to say that this is the first/initial
>> version, do I miss something?
>
> It was a joke - don't worry about it.
>
>>>> + .help = "disable\tdo not activate plugin\n"
>>>> + "verbose\tprint all debug infos\n",
>>>> +};
>>>> +static unsigned int arm64_scs_execute(void)
>>>> +{
>>>> + rtx_insn *insn;
>>>> + enum scs_state state = SCS_SEARCHING_FIRST_INSN;
>>>> +
>>>> + for (insn = get_insns(); insn; insn = NEXT_INSN(insn)) {
>>>> + rtx mark = NULL;
>>>> +
>>>> + switch (GET_CODE(insn)) {
>>>> + case NOTE:
>>>> + case BARRIER:
>>>> + case CODE_LABEL:
>>>> + case INSN:
>>>> + case DEBUG_INSN:
>>>> + case JUMP_INSN:
>>>> + case JUMP_TABLE_DATA:
>>>> + break;
>>>> + case CALL_INSN:
>>>> + if (SIBLING_CALL_P(insn)) {
>>>> + error(G_("Sibling call found in func:%s, file:%s\n"),
>>>> + get_name(current_function_decl),
>>>> + main_input_filename);
>>>> + gcc_unreachable();
>>>> + }
>>>
>>> Sibling calls are an important optimization, not only for performance
>>> but also for stack utilization, so this needs to be fixed. Can you
>>> elaborate on the issue you are working around here?
>>>
>> Since the ARM64 has disabled sibling calls (-fno-optimize-sibling-calls) by default,
>> there is almost no sibling call appear in the kernel I encountered.
>
> What do you mean this is disabled by default? Is that a compiler
> setting or a Linux setting?
It's a linux setting in aarch64 kernel.

In aarch64, since CONFIG_FRAME_POINTER is always selected, -fno-optimize-sibling-calls is
usually enable by default, and I think sibling calls rarely appear (I only encountered
it once in my cases from bsp's code):

./arch/arm64/Kconfig
config ARM64
...
select FRAME_POINTER

./Makefile
ifdef CONFIG_FRAME_POINTER
KBUILD_CFLAGS += -fno-omit-frame-pointer -fno-optimize-sibling-calls
...


>> So I did not provide support for it, and I will fix this issue in the next version.
>>>> + break;
>>>> + default:
>>>> + error(G_("Invalid rtx_insn seqs found with type:%s in func:%s, file:%s\n"),
>>>> + GET_RTX_NAME(GET_CODE(insn)),
>>>> + get_name(current_function_decl), main_input_filename);
>>>> + gcc_unreachable();
>>>> + break;
>>>> + }
>>>> + /* A function return insn was found */
>>>> + if (ANY_RETURN_P(PATTERN(insn))) {
>>>> + /* There should be an epilogue before 'RETURN' inst */
>>>> + if (GET_CODE(PATTERN(insn)) == RETURN) {
>>>> + gcc_assert(state == SCS_FOUND_ONE_EPILOGUE_NOTE);
>>>> + state = SCS_SEARCHING_FUNC_RETURN;
>>>> + }
>>>> +
>>>> + /* There is no epilogue before 'SIMPLE_RETURN' insn */
>>>> + if (GET_CODE(PATTERN(insn)) == SIMPLE_RETURN)
>>>> + gcc_assert(state == SCS_SEARCHING_FUNC_RETURN);
>>>
>>> These assert()s will crash the compiler if the RTL doesn't have quite
>>> the right structure, correct? Could we issue a warning instead, saying
>>> function 'x' could not be handled, and back out gracefully (i.e.,
>>> don't insert the push either)?
>>>
>> Sure, I think I need to dynamically mark all instrumented positions here,
>> and then confirm that the instruction sequence is correct before inserting in batches.
>
> Yes, that sounds more suitable.
>
>>>> +
>>>> + /* Insert scs pop instruction(s) before return insn */
>>>> + mark = gen_scs_pop(RESERVED_LOCATION_COUNT);
>>>> + emit_insn_before(mark, insn);
>>>> + }
>>>> + }
>>>> + return 0;
>>>> +}
>>>> +
>>>> +static tree handle_noscs_attribute(tree *node, tree name, tree args __unused, int flags,
>>>> + bool *no_add_attrs)
>>>> +{
>>>> + *no_add_attrs = true;
>>>> +
>>>> + gcc_assert(DECL_P(*node));
>>>> + switch (TREE_CODE(*node)) {
>>>> + default:
>>>> + error(G_("%qE attribute can be applies to function decl only (%qE)"), name, *node);
>>>> + gcc_unreachable();
>>>> +
>>>> + case FUNCTION_DECL: /* the attribute is only used for function declarations */
>>>> + break;
>>>> + }
>>>> +
>>>> + *no_add_attrs = false;
>>>
>>> I'm not familiar with this idiom: what is the purpose of setting this
>>> to true initially and then to false again when the expected flow
>>> through the function is to do nothing at all?
>>>
>> This is my mistake, at the beginning default case only return 0 directly after a warning;
>> At that time, if *no_add_attrs is true, the corresponding attribute will not be added to 'node',
>> and it means __noscs attribute can only be added for FUNCTION_DECL.
>> For now, *no_add_attrs = true; is useless, it should be deleted.
>>
>> But if, as you said, try to back out gracefully, is it better to report warning in the default case?
>
> error() just terminates the compile with an error, right? I think that is fine.
>
Yes. I got it.

>>>> + return NULL_TREE;
>>>> +}
>>>> +
>>>> +static void (*old_override_options_after_change)(void);
>>>> +
>>>> +static void scs_override_options_after_change(void)
>>>> +{
>>>> + if (old_override_options_after_change)
>>>> + old_override_options_after_change();
>>>> +
>>>> + flag_optimize_sibling_calls = 0;
>>>> +}
>>>> +
>>>> +static void callback_before_start_unit(void *gcc_data __unused, void *user_data __unused)
>>>> +{
>>>> + /* Turn off sibling call to avoid inserting duplicate scs pop codes */
>>>
>>> Sibling calls will restore x30 before the calk, right? So where do the
>>> duplicate pops come from?
>> a sibling call could be like:
>> stp x29, x30, [sp, #-xx]!
>> .......
>> ldp x29, x30, [sp], #xx
>> ---> p1
>> b callee
>> ldp x29, x30, [sp], #xx
>> ---> p2
>> ret
>>
>> What i mean here is if we need to insert, the scs pop code should be insert in both p1/p2,
>
> Yes, so you have to identify the 'b' insn as a function return so it
> is treated the same.
>
Thanks, let me try.
>>>
>>>> + old_override_options_after_change = targetm.override_options_after_change;
>>>> + targetm.override_options_after_change = scs_override_options_after_change;
>>>> +
>>>> + flag_optimize_sibling_calls = 0;
>>>
>>> Do we need this twice?
>> I think so, there are functions similar to push/pop in gcc (cl_optimization_restore/save)
>> * callback_before_start_unit is used to set zero during initialization
>> * scs_override_options_after_change is used to reset to 0 after a 'push' occurs
>
> OK
>
>>>> +}
>>>> +
>>>> +#define PASS_NAME arm64_scs
>>>> +#define TODO_FLAGS_FINISH (TODO_dump_func | TODO_verify_rtl_sharing)
>>>> +#include "gcc-generate-rtl-pass.h"
>>>> +
>>>> +__visible int plugin_init(struct plugin_name_args *plugin_info, struct plugin_gcc_version *version)
>>>> +{
>>>> + int i;
>>>> + const char * const plugin_name = plugin_info->base_name;
>>>> + const int argc = plugin_info->argc;
>>>> + const struct plugin_argument * const argv = plugin_info->argv;
>>>> + bool enable = true;
>>>> +
>>>> + PASS_INFO(arm64_scs, "shorten", 1, PASS_POS_INSERT_BEFORE);
>>>> +
>>>> + if (!plugin_default_version_check(version, &gcc_version)) {
>>>> + error(G_("Incompatible gcc/plugin versions"));
>>>> + return 1;
>>>> + }
>>>> +
>>>> + if (strncmp(lang_hooks.name, "GNU C", 5) && !strncmp(lang_hooks.name, "GNU C+", 6)) {
>>>> + inform(UNKNOWN_LOCATION, G_("%s supports C only, not %s"), plugin_name,
>>>> + lang_hooks.name);
>>>> + enable = false;
>>>> + }
>>>> +
>>>
>>> Do we need this check?
>> This code is copied from structleak_plugin.c, I misunderstood the meaning here, and I will delete it later
>
> OK. Kees should correct me if I'm wrong, but we use GCC in the kernel
> only to compile C files, so this check should be redundant.
>
>
>>>
>>>> + for (i = 0; i < argc; ++i) {
>>>> + if (!strcmp(argv[i].key, "disable")) {
>>>> + enable = false;
>>>> + continue;
>>>> + }
>>>> + if (!strcmp(argv[i].key, "verbose")) {
>>>> + verbose = true;
>>>> + continue;
>>>> + }
>>>> + error(G_("unknown option '-fplugin-arg-%s-%s'"), plugin_name, argv[i].key);
>>>> + }
>>>> +
>>>> + register_callback(plugin_name, PLUGIN_INFO, NULL, &arm64_scs_plugin_info);
>>>> +
>>>> + register_callback(plugin_name, PLUGIN_ATTRIBUTES, scs_register_attributes, NULL);
>>>> +
>>>> + if (!enable) {
>>>> + v_info("Plugin disabled for file:%s\n", main_input_filename);
>>>> + return 0;
>>>> + }
>>>> +
>>>> + register_callback(plugin_name, PLUGIN_START_UNIT, callback_before_start_unit, NULL);
>>>> +
>>>> + register_callback(plugin_name, PLUGIN_PASS_MANAGER_SETUP, NULL, &arm64_scs_pass_info);
>>>> +
>>>> + return 0;
>>>> +}
>>>> --
>>>> 2.7.4
>>>>

\
 
 \ /
  Last update: 2021-09-21 08:00    [W:2.597 / U:0.004 seconds]
©2003-2020 Jasper Spaans|hosted at Digital Ocean and TransIP|Read the blog|Advertise on this site