[PATCH 0/1] Fix exported symbol klp-relocation bug

Here is another klp-build bug reported downstream by Ben and reproduced
upstream by me:

Config
======

Default-ish config, note the KVM modules and allowing missing module NS
imports (more on that later):

  $ make defconfig
  $ ./scripts/config --file .config \
             --set-val CONFIG_FTRACE y \
             --set-val CONFIG_KALLSYMS_ALL y \
             --set-val CONFIG_FUNCTION_TRACER y \
             --set-val CONFIG_DYNAMIC_FTRACE y \
             --set-val CONFIG_DYNAMIC_DEBUG y \
             --set-val CONFIG_LIVEPATCH y \
             --set-val CONFIG_KVM m \
             --set-val CONFIG_KVM_INTEL m \
             --set-val CONFIG_MODULE_ALLOW_MISSING_NAMESPACE_IMPORTS y
  $ make olddefconfig


Base kernel and livepatch build
===============================

  $ make -j$(nproc)

Build a small patch to kvm :: mmu.c that introduces a call to
kvm_flush_remote_tlbs():

  $ cat minimal-kvm-repro.patch
  diff --git a/arch/x86/kvm/mmu/mmu.c b/arch/x86/kvm/mmu/mmu.c
  index 24fbc9ea502a..3f11b8c051ce 100644
  --- a/arch/x86/kvm/mmu/mmu.c
  +++ b/arch/x86/kvm/mmu/mmu.c
  @@ -2553,6 +2553,9 @@ static void __link_shadow_page(struct kvm *kvm,
                  drop_large_spte(kvm, sptep, flush);

          spte = make_nonleaf_spte(sp->spt, sp_ad_disabled(sp));
  +
  +       if (flush)
  +               kvm_flush_remote_tlbs(kvm);

          mmu_spte_set(sptep, spte);

Ignore the namespace complaint for now:

  $ ./scripts/livepatch/klp-build -T minimal-kvm-repro.patch
  Validating patch(es)
  Building original kernel
  Copying original object files
  Fixing patch(es)
  Building patched kernel
  Copying patched object files
  Generating original checksums
  Generating patched checksums
  Diffing objects
  arch/x86/kvm/kvm.o: changed function: __link_shadow_page
  Building patch module: livepatch-minimal-kvm-repro.ko
  WARNING: modpost: module livepatch-minimal-kvm-repro uses symbol kvm_flush_remote_tlbs from namespace module:kvm-intel, but does not import it.
  SUCCESS

klp-relocation inspection
=========================

Take a look at the generated klp-relocation symbols:

  $ readelf --wide --symbols livepatch-minimal-kvm-repro.ko | grep -o '.klp.sym.*'
  .klp.sym.kvm.kvm_flush_remote_tlbs_sptep,0
  .klp.sym.kvm.make_nonleaf_spte,0
  .klp.sym.kvm.drop_spte,0
  .klp.sym.arch/x86/kvm/kvm.kvm_flush_remote_tlbs,0        << "arch/x86/kvm/kvm" what?!
  .klp.sym.kvm.pte_list_add.isra.0,0
  .klp.sym.kvm.kvm_mmu_mark_parents_unsync,0

Oddly, the klp-relocation looks like the pathname / Module.symvers:

  $ grep kvm_flush_remote_tlbs Module.symvers
  0x00000000      kvm_flush_remote_tlbs   arch/x86/kvm/kvm        EXPORT_SYMBOL_GPL       module:kvm-intel
                                          ^^^^^^^^^^^^^^^^


klp-diff
========

In klp-diff.c :: clone_reloc_klp() there are two paths for grabbing a
symbol's module name, depending on whether it's been exported or not:

  	/*
  	 * Create the KLP symbol.
  	 */

  	if (export) {
  		sym_modname = export->mod;
  		sym_orig_name = export->sym;
  		sympos = 0;
  	} else {
  		sym_modname = find_modname(e);
  		if (!sym_modname)
  			return -1;

where the !export path calls find_modname() and __find_modname contains
some string manipulation at the end to form a simple, normalized kernel
module name:

  	for (char *c = name; *c; c++) {
  		if (*c == '/')
  			name = c + 1;
  		else if (*c == '-')
  			*c = '_';
  		else if (*c == '.') {
  			*c = '\0';
  			break;
  		}
  	}

but the export path assigns export->mod directly, which was originally
extracted from the Modules.symvers file in read_exports() with no such
string simplification.

AFAICT no where does klp-diff expect to handle export->mod as a path and
not a normalized module name.


Fix
===

With that, the attached patch extracts the string code from
__find_modname() into a helper function that both it and read_exports()
can call.

  $ ./scripts/livepatch/klp-build -T minimal-kvm-repro.patch
  Validating patch(es)
  Building original kernel
  Copying original object files
  Fixing patch(es)
  Building patched kernel
  Copying patched object files
  Generating original checksums
  Generating patched checksums
  Diffing objects
  arch/x86/kvm/kvm.o: changed function: __link_shadow_page
  Building patch module: livepatch-minimal-kvm-repro.ko
  WARNING: modpost: module livepatch-minimal-kvm-repro uses symbol kvm_flush_remote_tlbs from namespace module:kvm-intel, but does not import it.
  SUCCESS

  $ readelf --wide --symbols livepatch-minimal-kvm-repro.ko | grep -o '.klp.sym.*'
  .klp.sym.kvm.kvm_flush_remote_tlbs_sptep,0
  .klp.sym.kvm.make_nonleaf_spte,0
  .klp.sym.kvm.kvm_flush_remote_tlbs,0                     << fixed
  .klp.sym.kvm.drop_spte,0
  .klp.sym.kvm.pte_list_add.isra.0,0
  .klp.sym.kvm.kvm_mmu_mark_parents_unsync,0


Next: symbol namespaces
=======================

Now a harder question, I think, about symbol namespaces.  In the past,
kpatch-build had supported patching symbols in namespace where
MODULE_IMPORT_NS() is allowed.  Looking at kvm :: mmu.c
::kvm_flush_remote_tlbs(), that is annotated with
EXPORT_SYMBOL_FOR_KVM_INTERNAL() instead.

Should we make an effort to support klp-relocations / patching to this
use-case?

If modpost were to let klp-relocation symbols through, I *think*
(untested) that might be enough... but it seems like that may violate
the spirit of what the namespacing effort is trying to achieve.

Note that klp-post-link converts these symbols to SHN_LIVEPATCH before
the module is loaded, so the kernel module loader already skips them in
simplify_symbols() (see SHN_LIVEPATCH case).  AFAICT, the namespace
check in modpost is the only enforcement point, and it's checking a
symbol that will never be resolved through the normal module loading
path anyway.

Regards,

Joe Lawrence (1):
  objtool/klp-diff: normalize Module.symvers paths to module names

 tools/objtool/klp-diff.c | 42 ++++++++++++++++++++++++++++------------
 1 file changed, 30 insertions(+), 12 deletions(-)

-- 
2.54.0