objtool/klp: sympos/module/alternative/etc fixes

Testing klp-build with arm64 produced the following linker error during the original kernel build:

__notrim.1' referenced in section .klp.symid' of vmlinux.o: defined in discarded section `.no_trim_symbol' of vmlinux.o

symbol_get() puts a static __notrim[] in .no_trim_symbol, which GCC names __notrim.1, __notrim.2, etc. Two or more built-in translation units calling symbol_get() thus produce duplicate names, resulting in corresponding .klp.symid references which trigger the above error.

Add .no_trim_symbol to the discarded section list so its symbols don't get symids.

Note this issue is not specific to arm64: it just needs two built-in symbol_get() callers. arm64 trips over it easily because it has KVM always compiled in vmlinux, whereas on x86 it's typically a module.

Fixes 029223d30162 ("objtool/klp: Add .klp.symid for sympos disambiguation")

Signed-off-by Josh Poimboeuf <[email protected]>

tools/objtool/klp-diff.c +1 -0

index 492d7a012cffe..11e8f3ddbb0e6 100644
@@ -1627,6 +1627,7 @@ static int create_fake_symbols(struct elf *elf)
        for_each_reloc(sec->rsec, reloc) {
                unsigned long offset, size;
                struct reloc *next_reloc;
+               bool last = true;
 
                if (annotype(elf, sec, reloc) != ANNOTYPE_DATA_SPECIAL)
                        continue;

tools/objtool/klp-diff.c +0 -1

                if (create_fake_symbol(elf, reloc->sym->sec, offset, size))
--

2.54.0

Quoted 1 line · Josh Poimboeuf

[...]

Quoted 14 lines

tools/objtool/klp-diff.c +1 -0

index 492d7a012cffe..11e8f3ddbb0e6 100644
@@ -1627,6 +1627,7 @@ static int create_fake_symbols(struct elf *elf)
        for_each_reloc(sec->rsec, reloc) {
                unsigned long offset, size;
                struct reloc *next_reloc;
+               bool last = true;
 
                if (annotype(elf, sec, reloc) != ANNOTYPE_DATA_SPECIAL)
                        continue;

tools/objtool/klp-diff.c static int create_fake_symbols(struct elf *elf) · line 1650

Quoted 10 lines

tools/objtool/klp-diff.c +2 -1

@@ -1641,10 +1642,11 @@ static int create_fake_symbols(struct elf *elf)
                                continue;
 
                        size = reloc_addend(next_reloc) - offset;
+                       last = false;
                        break;
                }
 
-               if (!size)
+               if (last)
                        size = sec_size(reloc->sym->sec) - offset;

Some comments about "last" logic here can be very helpful.

Also, with the last flag, "size = 0;" before the for_each_reloc_continue() loop can be removed.

Other than these nitpicks:

Acked-by Song Liu <[email protected]>

Quoted 30 lines · Song Liu

Quoted 1 line · Josh Poimboeuf

[...]

Quoted 14 lines

tools/objtool/klp-diff.c +1 -0

index 492d7a012cffe..11e8f3ddbb0e6 100644
@@ -1627,6 +1627,7 @@ static int create_fake_symbols(struct elf *elf)
        for_each_reloc(sec->rsec, reloc) {
                unsigned long offset, size;
                struct reloc *next_reloc;
+               bool last = true;
 
                if (annotype(elf, sec, reloc) != ANNOTYPE_DATA_SPECIAL)
                        continue;

tools/objtool/klp-diff.c static int create_fake_symbols(struct elf *elf) · line 1650

Quoted 10 lines

tools/objtool/klp-diff.c +2 -1

@@ -1641,10 +1642,11 @@ static int create_fake_symbols(struct elf *elf)
                                continue;
 
                        size = reloc_addend(next_reloc) - offset;
+                       last = false;
                        break;
                }
 
-               if (!size)
+               if (last)
                        size = sec_size(reloc->sym->sec) - offset;

Some comments about "last" logic here can be very helpful.

Quoted 6 lines

Also, with the last flag, "size = 0;" before the for_each_reloc_continue() loop can be removed.

Other than these nitpicks:

Acked-by Song Liu <[email protected]>

How about this on top?

tools/objtool/klp-diff.c +8 -1

index 11e8f3ddbb0e6..38fae861d12c7 100644
@@ -1634,7 +1634,10 @@ static int create_fake_symbols(struct elf *elf)
 
                offset = reloc_addend(reloc);
 
-               size = 0;
+               /*
+                * Find the start of the next entry so the fake symbol size can
+                * be calculated.
+                */
                next_reloc = reloc;
                for_each_reloc_continue(sec->rsec, next_reloc) {
                        if (annotype(elf, sec, next_reloc) != ANNOTYPE_DATA_SPECIAL ||

@@ -1646,6 +1649,10 @@ static int create_fake_symbols(struct elf *elf)
                        break;
                }
 
+               /*
+                * If no next entry found, this is the last entry, so its size
+                * is from the current offset to the end of the section.
+                */
                if (last)
                        size = sec_size(reloc->sym->sec) - offset;
 

An x86 alternative with an empty replacement, e.g. the second entry of

ALTERNATIVE_2("orig", "repl", ft1, "", ft2)

has a replacementlen of zero. Its replacement offset still gets a relocation, but the label it points at is the end of the previous replacement, which is also the beginning of the *next* alternative's replacement. The value is meaningless; get_alt_entry() already ignores it for that reason.

klp diff doesn't ignore it. When such an alternative belongs to a changed function, cloning its relocations drags in the unrelated neighboring replacement, along with everything that replacement references. On an x86 clang/lto build an empty alternative in meminfo_proc_show() pulled in the replacement of an alternative in proc_kcore_init(), silently emitting a klp relocation against init text which has long since been freed by the time the patch is applied.

Add arch_alt_ignore_new_reloc() and skip such relocations when cloning. This has to be arch specific: on arm64 a zero-length replacement instead identifies an alternative callback, whose replacement offset points at the callback function and must be preserved.

Fixes dd590d4d57eb ("objtool/klp: Introduce klp diff subcommand for diffing object files")

Signed-off-by Josh Poimboeuf <[email protected]>

Quoted 19 lines · Josh Poimboeuf

An x86 alternative with an empty replacement, e.g. the second entry of

ALTERNATIVE_2("orig", "repl", ft1, "", ft2)

has a replacementlen of zero. Its replacement offset still gets a relocation, but the label it points at is the end of the previous replacement, which is also the beginning of the *next* alternative's replacement. The value is meaningless; get_alt_entry() already ignores it for that reason.

klp diff doesn't ignore it. When such an alternative belongs to a changed function, cloning its relocations drags in the unrelated neighboring replacement, along with everything that replacement references. On an x86 clang/lto build an empty alternative in meminfo_proc_show() pulled in the replacement of an alternative in proc_kcore_init(), silently emitting a klp relocation against init text which has long since been freed by the time the patch is applied.

Quoted 7 lines

Add arch_alt_ignore_new_reloc() and skip such relocations when cloning. This has to be arch specific: on arm64 a zero-length replacement instead identifies an alternative callback, whose replacement offset points at the callback function and must be preserved.

Fixes dd590d4d57eb ("objtool/klp: Introduce klp diff subcommand for diffing object files")

Signed-off-by Josh Poimboeuf <[email protected]>

The patch looks good to me.

Acked-by Song Liu <[email protected]>

Maybe we should add a __weak version of arch_alt_ignore_new_reloc(), but that can wait until we add arm64 support.

However, this reminds me the cross compile use case. With current arch_* functions, we cannot run klp-build on x86_64 build server for an arm64 kernel (right?). What's our plan with the cross-compile use cases?

Thank you for your contribution! Sashiko AI review found 1 potential issue(s) to consider:
- [High] The .init safety check is bypassed for exported module symbols, allowing livepatches to silently reference freed init memory. --

--- Patch [4]: [PATCH v2 4/7] objtool/klp: Explicitly disallow patching or referencing init code/data --- commit fe2fe6cc51b729f1eb6addc3f9e161e818ce482f
Author: Josh Poimboeuf <[email protected]>

objtool/klp: Explicitly disallow patching or referencing init code/data

This commit explicitly prevents the patching and referencing of init code and data to avoid edge cases. It introduces an init check in the sympos resolution to handle targets that might have their init section freed.

Quoted 4 lines

tools/objtool/klp-sympos.c +0 -0

index bbfae516d3395..dfca9dd746812 100644

[ ... ]

tools/objtool/klp-sympos.c unsigned long klp_find_sympos(struct elf *elf, struct symbol *sym) · line 388

Quoted 9 lines

tools/objtool/klp-sympos.c +5 -0

@@ -376,6 +381,11 @@ unsigned long klp_find_sympos(struct elf *elf, struct symbol *sym)
 	bool has_dup = false;
 	struct symbol *s;
 
+	if (is_init_sym(sym)) {
+		ERROR("%s: can't patch or reference init code/data", sym->name);
+		return ULONG_MAX;
+	}
+

[Severity: High]
Does this check adequately protect against exported module symbols?

Looking at clone_reloc_klp() in tools/objtool/klp-diff.c, if the symbol is exported, the code skips calling klp_find_sympos() entirely:

tools/objtool/klp-diff.c:clone_reloc_klp() {
    if (export) {
        sym_modname = export->mod;
        sym_orig_name = export->sym;
        sympos = 0;
    } else {
        sym_modname = find_modname(e);
        if (!sym_modname)
            return -1;

        sym_orig_name = patched_sym->twin->name;
        sympos = klp_find_sympos(e->orig, patched_sym->twin);
    ...
}

If a livepatch object references an exported init symbol from a module, would it bypass this new safety check and potentially result in a reference to freed init memory once the target module finishes initialization?

A klp relocation section is .klp.rela.<objname>.<secname>, where objname is the object being patched.

klp-build wrongly derives objname from where the referenced symbol lives, not where it's referenced. For a cross-module reference like patched can_isotp code calling can.ko's can_rx_unregister(), that gives .klp.rela.can..text rather than .klp.rela.can_isotp..text. Unless the patch happens to patch can.ko as well, the relocation never gets applied and the call goes off into the weeds.

Name the intermediate section __klp_relocs.<objname> so post-link can read the patched object's name from there.

Fixes dd590d4d57eb ("objtool/klp: Introduce klp diff subcommand for diffing object files")

Reported-by Joe Lawrence <[email protected]>

Link https://lore.kernel.org/[email protected] open here

Signed-off-by Josh Poimboeuf <[email protected]>

Exports are always global, so ignore local symbols in find_export().

Fixes dd590d4d57eb ("objtool/klp: Introduce klp diff subcommand for diffing object files")

Signed-off-by Josh Poimboeuf <[email protected]>

From: Joe Lawrence <[email protected]>

klp_reloc_needed() returns true for module exports to support late-module patching. However, clone_reloc_klp() unconditionally rejects symbols without a twin (i.e., new references added by the patch), even when the symbol is a known export from Module.symvers.

Relax the check: allow new references to exported symbols by only erroring on !twin when there is no export. The export metadata from Module.symvers provides sufficient context to emit the klp-relocation without a twin.

For a module export that isn't sufficient on its own though, as the resulting klp relocation will only be resolved at patch-enable time if the exporting module is loaded.

If the original (unpatched) module already depends on the exporting module, the dependency is safe: the module loader ensures the dependency is satisfied before the patched module can be loaded, so the klp relocation target will exist.

However, if the patch introduces a reference to a module that the original doesn't depend on, there is no such guarantee. The exporting module could be absent or could be unloaded at any time, leading to a relocation failure or use-after-free.

So also add a build-time check: when a new symbol reference (no twin) targets a module export, verify that the original module already has at least one UNDEF symbol resolving to that same exporting module. If not, error out with a diagnostic message.

Signed-off-by Joe Lawrence <[email protected]>

Signed-off-by Josh Poimboeuf <[email protected]>

v2:
- rebased on tip/master (first 6 patches were merged)
- dropped original patches 7-8 (will post followup)
- added .klp.symid fix

v1: https://lore.kernel.org/[email protected]

This consolidates fixes for the klp-build issues reported by Joe over the last several weeks, plus some more things I found while testing/reviewing.

Fun stuff like symbol resolution, module dependencies, alternatives.

Joe Lawrence (1):
objtool/klp: Allow new references to module exports

Josh Poimboeuf (6):
objtool/klp: Fix vmlinux .klp.symid link error for .no_trim_symbol
symbols
objtool/klp: Fix size of empty special section entries objtool/klp: Ignore replacement offset of empty x86 alternatives objtool/klp: Explicitly disallow patching or referencing init
code/data
objtool/klp: Fix cross-module klp relocation section naming objtool/klp: Don't match local symbols against exports

 tools/objtool/arch/x86/special.c        | 27 ++++++++++
 tools/objtool/include/objtool/klp.h     | 10 ++--
 tools/objtool/include/objtool/special.h |  7 +++
 tools/objtool/klp-diff.c                | 65 ++++++++++++++++++++++---
 tools/objtool/klp-post-link.c           | 53 +++++++++++---------
 tools/objtool/klp-symid.c               |  1 +
 tools/objtool/klp-sympos.c              | 10 ++++
 7 files changed, 141 insertions(+), 32 deletions(-)

Quoted 26 lines · Josh Poimboeuf

v2:
- rebased on tip/master (first 6 patches were merged)
- dropped original patches 7-8 (will post followup)
- added .klp.symid fix

v1: https://lore.kernel.org/[email protected]

This consolidates fixes for the klp-build issues reported by Joe over the last several weeks, plus some more things I found while testing/reviewing.

Fun stuff like symbol resolution, module dependencies, alternatives.

Joe Lawrence (1):
objtool/klp: Allow new references to module exports

Josh Poimboeuf (6):
objtool/klp: Fix vmlinux .klp.symid link error for .no_trim_symbol
symbols
objtool/klp: Fix size of empty special section entries objtool/klp: Ignore replacement offset of empty x86 alternatives objtool/klp: Explicitly disallow patching or referencing init
code/data
objtool/klp: Fix cross-module klp relocation section naming objtool/klp: Don't match local symbols against exports

Quoted 9 lines

 tools/objtool/arch/x86/special.c        | 27 ++++++++++
 tools/objtool/include/objtool/klp.h     | 10 ++--
 tools/objtool/include/objtool/special.h |  7 +++
 tools/objtool/klp-diff.c                | 65 ++++++++++++++++++++++---
 tools/objtool/klp-post-link.c           | 53 +++++++++++---------
 tools/objtool/klp-symid.c               |  1 +
 tools/objtool/klp-sympos.c              | 10 ++++
 7 files changed, 141 insertions(+), 32 deletions(-)

These changes (and follow-up comments) look pretty good (lightly tested). Could "objtool/klp: Fix vmlinux .klp.symid link error for .no_trim_symbol symbols" peel off and merge sooner since it fixes a commit just recently merged?

Though I think there is a still gap for EXPORT_SYMBOL_FOR_MODULES
symbols: those exported that way from vmlinux would presumably need a klp-relocation, but klp_reloc_needed() always returns false for all vmlinux exports. Maybe we need to parse the namespace column from Module.symvers to special case those? (At least, that is how I read the code, I don't have full repro test cases handy.)

This wouldn't block any of these patches, but if you are spinning a v3, we could consider tacking that on while we're here.