[PATCH 0/3] gcc-plugins/stackleak: Ignore .noinstr.text and .entry.text

[PATCH 0/3] gcc-plugins/stackleak: Ignore .noinstr.text and .entry.text

[Kees Cook]

Out of an abundance of caution, do not perform stack depth analysis on
.noinstr.text and .entry.text section functions, as it may be possible
that "current" is not sane.

Additionally, to verify results, the verbose mode is wired up the Kconfig,
and the string matching is refactored for correctness.

-Kees

Kees Cook (3):
  gcc-plugins/stackleak: Provide verbose mode
  gcc-plugins/stackleak: Exactly match strings instead of prefixes
  gcc-plugins/stackleak: Ignore .noinstr.text and .entry.text

 scripts/Makefile.gcc-plugins           |  2 ++
 scripts/gcc-plugins/stackleak_plugin.c | 29 ++++++++++++++++++++++----
 security/Kconfig.hardening             | 10 +++++++++
 3 files changed, 37 insertions(+), 4 deletions(-)

-- 
2.30.2

[PATCH 1/3] gcc-plugins/stackleak: Provide verbose mode

[Kees Cook]

In order to compare instrumentation between builds, make the verbose
mode of the plugin available during the build. This is rarely needed
(behind EXPERT) and very noisy (disabled for COMPILE_TEST).

Cc: Alexander Popov <alex.popov@linux.com>
Signed-off-by: Kees Cook <keescook@chromium.org>
---
 scripts/Makefile.gcc-plugins |  2 ++
 security/Kconfig.hardening   | 10 ++++++++++
 2 files changed, 12 insertions(+)

diff --git a/scripts/Makefile.gcc-plugins b/scripts/Makefile.gcc-plugins
index 1d16ca1b78c9..f67153b260c0 100644
--- a/scripts/Makefile.gcc-plugins
+++ b/scripts/Makefile.gcc-plugins
@@ -37,6 +37,8 @@ gcc-plugin-cflags-$(CONFIG_GCC_PLUGIN_STACKLEAK)		\
 		+= -fplugin-arg-stackleak_plugin-track-min-size=$(CONFIG_STACKLEAK_TRACK_MIN_SIZE)
 gcc-plugin-cflags-$(CONFIG_GCC_PLUGIN_STACKLEAK)		\
 		+= -fplugin-arg-stackleak_plugin-arch=$(SRCARCH)
+gcc-plugin-cflags-$(CONFIG_GCC_PLUGIN_STACKLEAK_VERBOSE)	\
+		+= -fplugin-arg-stackleak_plugin-verbose
 ifdef CONFIG_GCC_PLUGIN_STACKLEAK
     DISABLE_STACKLEAK_PLUGIN += -fplugin-arg-stackleak_plugin-disable
 endif
diff --git a/security/Kconfig.hardening b/security/Kconfig.hardening
index d051f8ceefdd..ded4d7c0d132 100644
--- a/security/Kconfig.hardening
+++ b/security/Kconfig.hardening
@@ -174,6 +174,16 @@ config GCC_PLUGIN_STACKLEAK
 	   * https://grsecurity.net/
 	   * https://pax.grsecurity.net/
 
+config GCC_PLUGIN_STACKLEAK_VERBOSE
+	bool "Report stack depth analysis instrumentation" if EXPERT
+	depends on GCC_PLUGIN_STACKLEAK
+	depends on !COMPILE_TEST	# too noisy
+	help
+	  This option will cause a warning to be printed each time the
+	  stackleak plugin finds a function it thinks needs to be
+	  instrumented. This is useful for comparing coverage between
+	  builds.
+
 config STACKLEAK_TRACK_MIN_SIZE
 	int "Minimum stack frame size of functions tracked by STACKLEAK"
 	default 100
-- 
2.30.2

[PATCH 2/3] gcc-plugins/stackleak: Exactly match strings instead of prefixes

[Kees Cook]

Since STRING_CST may not be NUL terminated, strncmp() was used for check
for equality. However, this may lead to mismatches for longer section
names where the start matches the tested-for string. Test for exact
equality by checking for the presences of NUL termination.

Cc: Alexander Popov <alex.popov@linux.com>
Signed-off-by: Kees Cook <keescook@chromium.org>
---
 scripts/gcc-plugins/stackleak_plugin.c | 25 +++++++++++++++++++++----
 1 file changed, 21 insertions(+), 4 deletions(-)

diff --git a/scripts/gcc-plugins/stackleak_plugin.c b/scripts/gcc-plugins/stackleak_plugin.c
index e9db7dcb3e5f..623bcad6d0c7 100644
--- a/scripts/gcc-plugins/stackleak_plugin.c
+++ b/scripts/gcc-plugins/stackleak_plugin.c
@@ -429,6 +429,23 @@ static unsigned int stackleak_cleanup_execute(void)
 	return 0;
 }
 
+/*
+ * STRING_CST may or may not be NUL terminated:
+ * https://gcc.gnu.org/onlinedocs/gccint/Constant-expressions.html
+ */
+static inline bool string_equal(tree node, const char *string, int length)
+{
+	if (TREE_STRING_LENGTH(node) < length)
+		return false;
+	if (TREE_STRING_LENGTH(node) > length + 1)
+		return false;
+	if (TREE_STRING_LENGTH(node) == length + 1 &&
+	    TREE_STRING_POINTER(node)[length] != '\0')
+		return false;
+	return !strncmp(TREE_STRING_POINTER(node), string, length);
+}
+#define STRING_EQUAL(node, str)	string_equal(node, str, strlen(str))
+
 static bool stackleak_gate(void)
 {
 	tree section;
@@ -438,13 +455,13 @@ static bool stackleak_gate(void)
 	if (section && TREE_VALUE(section)) {
 		section = TREE_VALUE(TREE_VALUE(section));
 
-		if (!strncmp(TREE_STRING_POINTER(section), ".init.text", 10))
+		if (STRING_EQUAL(section, ".init.text"))
 			return false;
-		if (!strncmp(TREE_STRING_POINTER(section), ".devinit.text", 13))
+		if (STRING_EQUAL(section, ".devinit.text"))
 			return false;
-		if (!strncmp(TREE_STRING_POINTER(section), ".cpuinit.text", 13))
+		if (STRING_EQUAL(section, ".cpuinit.text"))
 			return false;
-		if (!strncmp(TREE_STRING_POINTER(section), ".meminit.text", 13))
+		if (STRING_EQUAL(section, ".meminit.text"))
 			return false;
 	}
 
-- 
2.30.2

[PATCH 3/3] gcc-plugins/stackleak: Ignore .noinstr.text and .entry.text

[Kees Cook]

The .noinstr.text section functions may not have "current()" sanely
available. Similarly true for .entry.text, though such a check is
currently redundant. Add a check for both. In an x86_64 defconfig build,
the following functions no longer receive stackleak instrumentation:

	__do_fast_syscall_32()
	do_int80_syscall_32()
	do_machine_check()
	do_syscall_64()
	exc_general_protection()
	fixup_bad_iret()

Suggested-by: Peter Zijlstra <peterz@infradead.org>
Cc: Alexander Popov <alex.popov@linux.com>
Signed-off-by: Kees Cook <keescook@chromium.org>
---
 scripts/gcc-plugins/stackleak_plugin.c | 4 ++++
 1 file changed, 4 insertions(+)

diff --git a/scripts/gcc-plugins/stackleak_plugin.c b/scripts/gcc-plugins/stackleak_plugin.c
index 623bcad6d0c7..c8dc7fe4f959 100644
--- a/scripts/gcc-plugins/stackleak_plugin.c
+++ b/scripts/gcc-plugins/stackleak_plugin.c
@@ -463,6 +463,10 @@ static bool stackleak_gate(void)
 			return false;
 		if (STRING_EQUAL(section, ".meminit.text"))
 			return false;
+		if (STRING_EQUAL(section, ".noinstr.text"))
+			return false;
+		if (STRING_EQUAL(section, ".entry.text"))
+			return false;
 	}
 
 	return track_frame_size >= 0;
-- 
2.30.2

Re: [PATCH 2/3] gcc-plugins/stackleak: Exactly match strings instead of prefixes

[Linus Torvalds]

On Sun, Feb 6, 2022 at 9:45 AM Kees Cook <keescook@chromium.org> wrote:
>
> +       return !strncmp(TREE_STRING_POINTER(node), string, length);

Why is this "strncmp()"? That makes no sense when you've just checked
the exact lengths of both sides.

You're not comparing strings any more, you've already checked the end
of the string - you are comparing memory contents.

So make it just do a "memcmp()".

> +#define STRING_EQUAL(node, str)        string_equal(node, str, strlen(str))

.. and please change this name too, since it's not comparing two
strings. The first argument is something else entirely.

It's checking the node value of a section, give it some name related to that.

I do also get the feeling that the nodes should actually be checked to
be a STRING_CST rather than these blind TREE_VALUE() following things,
but I don't really know the rules for gcc plugin internals very well -
or at all, really.

                   Linus

Re: [PATCH 2/3] gcc-plugins/stackleak: Exactly match strings instead of prefixes

[Kees Cook]

On Sun, Feb 06, 2022 at 10:34:11AM -0800, Linus Torvalds wrote:
> On Sun, Feb 6, 2022 at 9:45 AM Kees Cook <keescook@chromium.org> wrote:
> >
> > +       return !strncmp(TREE_STRING_POINTER(node), string, length);
> 
> Why is this "strncmp()"? That makes no sense when you've just checked
> the exact lengths of both sides.
> 
> You're not comparing strings any more, you've already checked the end
> of the string - you are comparing memory contents.
> 
> So make it just do a "memcmp()".

Yeah, good point. I'll change this for v2, pending more feedback.

> > +#define STRING_EQUAL(node, str)        string_equal(node, str, strlen(str))
> 
> .. and please change this name too, since it's not comparing two
> strings. The first argument is something else entirely.
> 
> It's checking the node value of a section, give it some name related to that.

Technically, yes. The naming bikeshed here is odd since it's called
"STRING" by gcc internals, and it *might* be a "C string", etc etc. I'll
rename it...

> I do also get the feeling that the nodes should actually be checked to
> be a STRING_CST rather than these blind TREE_VALUE() following things,
> but I don't really know the rules for gcc plugin internals very well -
> or at all, really.

I'll double-check this, but if it's not a STRING_CST something else has
gone very wrong already. But I'm a fan of robustness, so sure. :)

-Kees

-- 
Kees Cook