linux-kernel - Re: [PATCH v2 00/62] objtool,livepatch: klp-build livepatch module generation

lists.openwall.net		lists / announce owl-users owl-dev john-users john-dev passwdqc-users yescrypt popa3d-users / oss-security kernel-hardening musl sabotage tlsify passwords / crypt-dev xvendor / Bugtraq Full-Disclosure linux-kernel linux-netdev linux-ext4 linux-hardening linux-cve-announce PHC
Open Source and information security mailing list archives

Hash Suite: Windows password security audit tool. GUI, reports in PDF.

[<prev] [next>] [<thread-prev] [day] [month] [year] [list]

Message-ID: <2mmkbpkj2b3a7qxrkk32vyg7hiwuo3dh2blispfgdb6u7wrysx@xbcthy36wx6q>
Date: Thu, 12 Jun 2025 13:56:37 -0700
From: Josh Poimboeuf <jpoimboe@...nel.org>
To: x86@...nel.org
Cc: linux-kernel@...r.kernel.org, Petr Mladek <pmladek@...e.com>, 
	Miroslav Benes <mbenes@...e.cz>, Joe Lawrence <joe.lawrence@...hat.com>, 
	live-patching@...r.kernel.org, Song Liu <song@...nel.org>, laokz <laokz@...mail.com>, 
	Jiri Kosina <jikos@...nel.org>, Marcos Paulo de Souza <mpdesouza@...e.com>, 
	Weinan Liu <wnliu@...gle.com>, Fazla Mehrab <a.mehrab@...edance.com>, 
	Chen Zhongjin <chenzhongjin@...wei.com>, Puranjay Mohan <puranjay@...nel.org>
Subject: Re: [PATCH v2 00/62] objtool,livepatch: klp-build livepatch module
 generation

On Fri, May 09, 2025 at 01:16:24PM -0700, Josh Poimboeuf wrote:
> I've tested with a variety of patches on defconfig and Fedora-config
> kernels with both GCC and Clang.
> 
> These patches can also be found at:
> 
>   git://git.kernel.org/pub/scm/linux/kernel/git/jpoimboe/linux.git klp-build-v2
> 
> Please test!

I found a nasty bug while trying to patch copy_process().  The wrong
version of __refcount_add.constprop.0() was being called due to a bad
klp rela sympos value, causing refcount overflow/UAF warnings and hangs.

The problem was a mismatch between the sympos order of the vmlinux.o
archive (which klp-diff uses) and the final vmlinux.

The linker script manually emits .text.unlikely before .text instead of
emitting in them in section table order.  So if a function name exists
in both sections, the calculated sympos (based on vmlinux.o) is wrong.

In my test kernel with GCC 14, only 25 out of 136,931 functions (0.018%)
have this problem.  It was my lucky day.

The below hack fixes it by starting the sympos counting with
.text.unlikely*.  It's a bit fragile, but it works fine for now.

I'm thinking the final fix would involve adding a checksum field to
kallsyms.  Then sympos would no longer be needed.  But that will need to
come later.

diff --git a/tools/include/linux/string.h b/tools/include/linux/string.h
index 8499f509f03e..51ad3cf4fa82 100644
--- a/tools/include/linux/string.h
+++ b/tools/include/linux/string.h
@@ -44,6 +44,20 @@ static inline bool strstarts(const char *str, const char *prefix)
 	return strncmp(str, prefix, strlen(prefix)) == 0;
 }
 
+/*
+ * Checks if a string ends with another.
+ */
+static inline bool str_ends_with(const char *str, const char *substr)
+{
+	size_t len = strlen(str);
+	size_t sublen = strlen(substr);
+
+	if (sublen > len)
+		return false;
+
+	return !strcmp(str + len - sublen, substr);
+}
+
 extern char * __must_check skip_spaces(const char *);
 
 extern char *strim(char *);
diff --git a/tools/objtool/check.c b/tools/objtool/check.c
index b5494b5ca78f..47ee010a7852 100644
--- a/tools/objtool/check.c
+++ b/tools/objtool/check.c
@@ -187,20 +187,6 @@ static bool is_sibling_call(struct instruction *insn)
 	return (is_static_jump(insn) && insn_call_dest(insn));
 }
 
-/*
- * Checks if a string ends with another.
- */
-static bool str_ends_with(const char *s, const char *sub)
-{
-	const int slen = strlen(s);
-	const int sublen = strlen(sub);
-
-	if (sublen > slen)
-		return 0;
-
-	return !memcmp(s + slen - sublen, sub, sublen);
-}
-
 /*
  * Checks if a function is a Rust "noreturn" one.
  */
diff --git a/tools/objtool/klp-diff.c b/tools/objtool/klp-diff.c
index 5276964ef123..0290cbc90c16 100644
--- a/tools/objtool/klp-diff.c
+++ b/tools/objtool/klp-diff.c
@@ -439,6 +439,7 @@ static int correlate_symbols(struct elfs *e)
 /* "sympos" is used by livepatch to disambiguate duplicate symbol names */
 static unsigned long find_sympos(struct elf *elf, struct symbol *sym)
 {
+	bool vmlinux = str_ends_with(objname, "vmlinux.o");
 	unsigned long sympos = 0, nr_matches = 0;
 	bool has_dup = false;
 	struct symbol *s;
@@ -446,13 +447,43 @@ static unsigned long find_sympos(struct elf *elf, struct symbol *sym)
 	if (sym->bind != STB_LOCAL)
 		return 0;
 
-	for_each_sym(elf, s) {
-		if (!strcmp(s->name, sym->name)) {
-			nr_matches++;
-			if (s == sym)
-				sympos = nr_matches;
-			else
-				has_dup = true;
+	if (vmlinux && sym->type == STT_FUNC) {
+		/*
+		 * HACK: Unfortunately, symbol ordering can differ between
+		 * vmlinux.o and vmlinux due to the linker script emitting
+		 * .text.unlikely* before .text*.  Count .text.unlikely* first.
+		 *
+		 * TODO: Disambiguate symbols more reliably (checksums?)
+		 */
+		for_each_sym(elf, s) {
+			if (strstarts(s->sec->name, ".text.unlikely") &&
+			    !strcmp(s->name, sym->name)) {
+				nr_matches++;
+				if (s == sym)
+					sympos = nr_matches;
+				else
+					has_dup = true;
+			}
+		}
+		for_each_sym(elf, s) {
+			if (!strstarts(s->sec->name, ".text.unlikely") &&
+			    !strcmp(s->name, sym->name)) {
+				nr_matches++;
+				if (s == sym)
+					sympos = nr_matches;
+				else
+					has_dup = true;
+			}
+		}
+	} else {
+		for_each_sym(elf, s) {
+			if (!strcmp(s->name, sym->name)) {
+				nr_matches++;
+				if (s == sym)
+					sympos = nr_matches;
+				else
+					has_dup = true;
+			}
 		}
 	}