On 31.05.2022 21:13, Kees Cook wrote:

On Fri, May 27, 2022 at 02:25:12AM +0300, Alexander Popov wrote:

quoted

On 24.05.2022 16:31, Mark Rutland wrote:

quoted

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

quoted

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?

quoted

[...]

quoted

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



_______________________________________________
linux-arm-kernel mailing list
linux-arm-kernel@lists.infradead.org
http://lists.infradead.org/mailman/listinfo/linux-arm-kernel