Re: [PATCH v2 07/13] stackleak: rework poison scanning

[Kees Cook <keescook@chromium.org>]

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