| Message ID | 20220206174508.2425076-3-keescook@chromium.org (mailing list archive) |
|---|---|
| State | Changes Requested |
| Headers | show |
| Series | gcc-plugins/stackleak: Ignore .noinstr.text and .entry.text | expand |
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
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
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; }
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(-)