Messages in this thread | | | Date | Tue, 31 May 2022 11:13:44 -0700 | From | Kees Cook <> | Subject | Re: [PATCH v2 07/13] stackleak: rework poison scanning |
| |
On Fri, May 27, 2022 at 02:25:12AM +0300, Alexander Popov wrote: > On 24.05.2022 16:31, Mark Rutland wrote: > > [...] > > It's also worth noting that `noinstr` code will also not be instrumented > > regardless of frame size -- we might want some build-time assertion for those. > > I developed a trick that shows noinstr functions that stackleak would like to instrument: > > diff --git a/scripts/gcc-plugins/stackleak_plugin.c b/scripts/gcc-plugins/stackleak_plugin.c > index 42f0252ee2a4..6db748d44957 100644 > --- a/scripts/gcc-plugins/stackleak_plugin.c > +++ b/scripts/gcc-plugins/stackleak_plugin.c > @@ -397,6 +397,9 @@ static unsigned int stackleak_cleanup_execute(void) > const char *fn = DECL_NAME_POINTER(current_function_decl); > bool removed = false; > > + if (verbose) > + fprintf(stderr, "stackleak: I see noinstr function %s()\n", fn); > + > /* > * Leave stack tracking in functions that call alloca(). > * Additional case: > @@ -464,12 +467,12 @@ static bool stackleak_gate(void) > if (STRING_EQUAL(section, ".meminit.text")) > return false; > if (STRING_EQUAL(section, ".noinstr.text")) > - return false; > + return true; > if (STRING_EQUAL(section, ".entry.text")) > return false; > } > > - return track_frame_size >= 0; > + return false; > } > > /* Build the function declaration for stackleak_track_stack() */ > @@ -589,8 +592,6 @@ __visible int plugin_init(struct plugin_name_args *plugin_info, > build_for_x86 = true; > } else if (!strcmp(argv[i].key, "disable")) { > disable = true; > - } else if (!strcmp(argv[i].key, "verbose")) { > - verbose = true; > } else { > error(G_("unknown option '-fplugin-arg-%s-%s'"), > plugin_name, argv[i].key); > @@ -598,6 +599,8 @@ __visible int plugin_init(struct plugin_name_args *plugin_info, > } > } > > + verbose = true; > + > if (disable) { > if (verbose) > fprintf(stderr, "stackleak: disabled for this translation unit\n"); > > > Building defconfig for x86_64 gives this: > > stackleak: I see noinstr function __do_fast_syscall_32() > stackleak: instrument __do_fast_syscall_32(): calls_alloca > -- > stackleak: I see noinstr function do_syscall_64() > stackleak: instrument do_syscall_64(): calls_alloca > -- > stackleak: I see noinstr function do_int80_syscall_32() > stackleak: instrument do_int80_syscall_32(): calls_alloca
As you say, these are from RANDOMIZE_KSTACK_OFFSET, and are around bounds-checked, and should already be getting wiped since these will call into deeper (non-noinst) functions.
> stackleak: I see noinstr function do_machine_check() > stackleak: instrument do_machine_check() > -- > stackleak: I see noinstr function exc_general_protection() > stackleak: instrument exc_general_protection() > -- > stackleak: I see noinstr function fixup_bad_iret() > stackleak: instrument fixup_bad_iret() > > > The cases with calls_alloca are caused by CONFIG_RANDOMIZE_KSTACK_OFFSET=y. > Kees knows about that peculiarity. > > Other cases are noinstr functions with large stack frame: > do_machine_check(), exc_general_protection(), and fixup_bad_iret(). > > I think adding a build-time assertion is not possible, since it would break > building the kernel.
Do these functions share the syscall behavior of always calling into non-noinst functions that _do_ have stack depth instrumentation?
> [...] > > In security/Kconfig.hardening we have: > > > > | config STACKLEAK_TRACK_MIN_SIZE > > | int "Minimum stack frame size of functions tracked by STACKLEAK" > > | default 100 > > | range 0 4096 > > | depends on GCC_PLUGIN_STACKLEAK > > | help > > | The STACKLEAK gcc plugin instruments the kernel code for tracking > > | the lowest border of the kernel stack (and for some other purposes). > > | It inserts the stackleak_track_stack() call for the functions with > > | a stack frame size greater than or equal to this parameter. > > | If unsure, leave the default value 100. > > > > ... where the vast majority of that range is going to lead to a BUILD_BUG(). > > Honestly, I don't like the idea of having the STACKLEAK_TRACK_MIN_SIZE option in the Kconfig. > > I was forced by the maintainers to introduce it when I was working on the stackleak patchset. > > How about dropping CONFIG_STACKLEAK_TRACK_MIN_SIZE from Kconfig? > > That would also allow to drop this build-time assertion.
Should this be arch-specific? (i.e. just make it a per-arch Kconfig default, instead of user-selectable into weird values?)
-- Kees Cook
| |