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;
+ }
}
}
