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

[Alexander Popov]

On 31.05.2022 21:13, Kees Cook wrote:
> 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.

Kees, it crossed my mind that for correct stack erasing the kernel with 
RANDOMIZE_KSTACK_OFFSET needs at least one stackleak_track_stack() call during 
the syscall handling.

Otherwise current->lowest_stack would point to the stack address where no stack 
frame was placed because of alloca with random size.

Am I right?

How about calling stackleak_track_stack() explicitly after the kernel stack 
randomization?


> > 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?

This is a right question.

I can't say for sure, but it looks like do_machine_check(), 
exc_general_protection() and fixup_bad_iret() do some low-level exception/trap 
handling and don't affect syscall handling. Do you agree?

> > [...]
> > > 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?)

I don't think CONFIG_STACKLEAK_TRACK_MIN_SIZE is arch-specific, since 
STACKLEAK_SEARCH_DEPTH is the same for all architectures that support stackleak.

Best regards,
Alexander