* [PATCH 1/3] gcc-plugins/stackleak: Provide verbose mode
2022-02-06 17:45 [PATCH 0/3] gcc-plugins/stackleak: Ignore .noinstr.text and .entry.text Kees Cook
@ 2022-02-06 17:45 ` Kees Cook
2022-02-06 17:45 ` [PATCH 2/3] gcc-plugins/stackleak: Exactly match strings instead of prefixes Kees Cook
2022-02-06 17:45 ` [PATCH 3/3] gcc-plugins/stackleak: Ignore .noinstr.text and .entry.text Kees Cook
2 siblings, 0 replies; 6+ messages in thread
From: Kees Cook @ 2022-02-06 17:45 UTC (permalink / raw)
To: Alexander Popov
Cc: Kees Cook, Peter Zijlstra, Linus Torvalds, Thomas Gleixner,
Josh Poimboeuf, Borislav Petkov, Masahiro Yamada, linux-kernel,
linux-hardening
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
^ permalink raw reply related [flat|nested] 6+ messages in thread
* [PATCH 2/3] gcc-plugins/stackleak: Exactly match strings instead of prefixes
2022-02-06 17:45 [PATCH 0/3] gcc-plugins/stackleak: Ignore .noinstr.text and .entry.text Kees Cook
2022-02-06 17:45 ` [PATCH 1/3] gcc-plugins/stackleak: Provide verbose mode Kees Cook
@ 2022-02-06 17:45 ` Kees Cook
2022-02-06 18:34 ` Linus Torvalds
2022-02-06 17:45 ` [PATCH 3/3] gcc-plugins/stackleak: Ignore .noinstr.text and .entry.text Kees Cook
2 siblings, 1 reply; 6+ messages in thread
From: Kees Cook @ 2022-02-06 17:45 UTC (permalink / raw)
To: Alexander Popov
Cc: Kees Cook, Peter Zijlstra, Linus Torvalds, Thomas Gleixner,
Josh Poimboeuf, Borislav Petkov, Masahiro Yamada, linux-kernel,
linux-hardening
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
^ permalink raw reply related [flat|nested] 6+ messages in thread
* Re: [PATCH 2/3] gcc-plugins/stackleak: Exactly match strings instead of prefixes
2022-02-06 17:45 ` [PATCH 2/3] gcc-plugins/stackleak: Exactly match strings instead of prefixes Kees Cook
@ 2022-02-06 18:34 ` Linus Torvalds
2022-02-06 18:49 ` Kees Cook
0 siblings, 1 reply; 6+ messages in thread
From: Linus Torvalds @ 2022-02-06 18:34 UTC (permalink / raw)
To: Kees Cook
Cc: Alexander Popov, Peter Zijlstra, Thomas Gleixner, Josh Poimboeuf,
Borislav Petkov, Masahiro Yamada, linux-kernel, linux-hardening
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
^ permalink raw reply [flat|nested] 6+ messages in thread
* Re: [PATCH 2/3] gcc-plugins/stackleak: Exactly match strings instead of prefixes
2022-02-06 18:34 ` Linus Torvalds
@ 2022-02-06 18:49 ` Kees Cook
0 siblings, 0 replies; 6+ messages in thread
From: Kees Cook @ 2022-02-06 18:49 UTC (permalink / raw)
To: Linus Torvalds
Cc: Alexander Popov, Peter Zijlstra, Thomas Gleixner, Josh Poimboeuf,
Borislav Petkov, Masahiro Yamada, linux-kernel, linux-hardening
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
^ permalink raw reply [flat|nested] 6+ messages in thread
* [PATCH 3/3] gcc-plugins/stackleak: Ignore .noinstr.text and .entry.text
2022-02-06 17:45 [PATCH 0/3] gcc-plugins/stackleak: Ignore .noinstr.text and .entry.text Kees Cook
2022-02-06 17:45 ` [PATCH 1/3] gcc-plugins/stackleak: Provide verbose mode Kees Cook
2022-02-06 17:45 ` [PATCH 2/3] gcc-plugins/stackleak: Exactly match strings instead of prefixes Kees Cook
@ 2022-02-06 17:45 ` Kees Cook
2 siblings, 0 replies; 6+ messages in thread
From: Kees Cook @ 2022-02-06 17:45 UTC (permalink / raw)
To: Alexander Popov
Cc: Kees Cook, Peter Zijlstra, Linus Torvalds, Thomas Gleixner,
Josh Poimboeuf, Borislav Petkov, Masahiro Yamada, linux-kernel,
linux-hardening
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
^ permalink raw reply related [flat|nested] 6+ messages in thread