KLP relocations are livepatch-specific relocations which are applied to
a KLP module's text or data. They exist for two reasons:
1) Unexported symbols: replacement functions often need to access
unexported symbols (e.g. static functions), which "normal"
relocations don't allow.
2) Late module patching: this is the ability for a KLP module to
bypass normal module dependencies, such that the KLP module can be
loaded *before* a to-be-patched module. This means that
relocations which need to access symbols in the to-be-patched
module might need to be applied to the KLP module well after it has
been loaded.
Non-late-patched KLP relocations are applied from the KLP module's init
function. That usually works fine, unless the patched code wants to use
alternatives, paravirt patching, jump tables, or some other special
section which needs relocations. Then we run into ordering issues and
crashes.
In order for those special sections to work properly, the KLP
relocations should be applied *before* the special section init code
runs, such as apply_paravirt(), apply_alternatives(), or
jump_label_apply_nops().
You might think the obvious solution would be to move the KLP relocation
initialization earlier, but it's not necessarily that simple. The
problem is the above-mentioned late module patching, for which KLP
relocations can get applied well after the KLP module is loaded.
To "fix" this issue in the past, we created .klp.arch sections:
.klp.arch.{module}..altinstructions
.klp.arch.{module}..parainstructions
Those sections allow KLP late module patching code to call
apply_paravirt() and apply_alternatives() after the module-specific KLP
relocations (.klp.rela.{module}.{section}) have been applied.
But that has a lot of drawbacks, including code complexity, the need for
arch-specific code, and the (per-arch) danger that we missed some
special section -- for example the __jump_table section which is used
for jump labels.
It turns out there's a simpler and more functional approach. There are
two kinds of KLP relocation sections:
1) vmlinux-specific KLP relocation sections
.klp.rela.vmlinux.{sec}
These are relocations (applied to the KLP module) which reference
unexported vmlinux symbols.
2) module-specific KLP relocation sections
.klp.rela.{module}.{sec}:
These are relocations (applied to the KLP module) which reference
unexported or exported module symbols.
Up until now, these have been treated the same. However, they're
inherently different.
Because of late module patching, module-specific KLP relocations can be
applied very late, thus they can create the ordering headaches described
above.
But vmlinux-specific KLP relocations don't have that problem. There's
nothing to prevent them from being applied earlier. So apply them at
the same time as normal relocations, when the KLP module is being
loaded.
This means that for vmlinux-specific KLP relocations, we no longer have
any ordering issues. vmlinux-referencing jump labels, alternatives, and
paravirt patching will work automatically, without the need for the
.klp.arch hacks.
All that said, for module-specific KLP relocations, the ordering
problems still exist and we *do* still need .klp.arch. Or do we? Stay
tuned.
Suggested-by: Peter Zijlstra <[email protected]>
Acked-by: Peter Zijlstra (Intel) <[email protected]>
Signed-off-by: Josh Poimboeuf <[email protected]>
---
include/linux/livepatch.h | 14 +++++++
kernel/livepatch/core.c | 86 ++++++++++++++++++++++++++-------------
kernel/module.c | 9 ++--
3 files changed, 77 insertions(+), 32 deletions(-)
diff --git a/include/linux/livepatch.h b/include/linux/livepatch.h
index e894e74905f3..85fc23759dc1 100644
--- a/include/linux/livepatch.h
+++ b/include/linux/livepatch.h
@@ -234,6 +234,11 @@ void klp_shadow_free_all(unsigned long id, klp_shadow_dtor_t dtor);
struct klp_state *klp_get_state(struct klp_patch *patch, unsigned long id);
struct klp_state *klp_get_prev_state(unsigned long id);
+int klp_write_relocations(Elf_Ehdr *ehdr, Elf_Shdr *sechdrs,
+ const char *shstrtab, const char *strtab,
+ unsigned int symindex, struct module *pmod,
+ const char *objname);
+
#else /* !CONFIG_LIVEPATCH */
static inline int klp_module_coming(struct module *mod) { return 0; }
@@ -242,6 +247,15 @@ static inline bool klp_patch_pending(struct task_struct *task) { return false; }
static inline void klp_update_patch_state(struct task_struct *task) {}
static inline void klp_copy_process(struct task_struct *child) {}
+static inline
+int klp_write_relocations(Elf_Ehdr *ehdr, Elf_Shdr *sechdrs,
+ const char *shstrtab, const char *strtab,
+ unsigned int symindex, struct module *pmod,
+ const char *objname)
+{
+ return 0;
+}
+
#endif /* CONFIG_LIVEPATCH */
#endif /* _LINUX_LIVEPATCH_H_ */
diff --git a/kernel/livepatch/core.c b/kernel/livepatch/core.c
index 40cfac8156fd..5fda3afc0285 100644
--- a/kernel/livepatch/core.c
+++ b/kernel/livepatch/core.c
@@ -191,12 +191,12 @@ static int klp_find_object_symbol(const char *objname, const char *name,
return -EINVAL;
}
-static int klp_resolve_symbols(Elf_Shdr *relasec, struct module *pmod)
+static int klp_resolve_symbols(Elf64_Shdr *sechdrs, const char *strtab,
+ unsigned int symndx, Elf_Shdr *relasec)
{
int i, cnt, vmlinux, ret;
char objname[MODULE_NAME_LEN];
char symname[KSYM_NAME_LEN];
- char *strtab = pmod->core_kallsyms.strtab;
Elf_Rela *relas;
Elf_Sym *sym;
unsigned long sympos, addr;
@@ -216,7 +216,7 @@ static int klp_resolve_symbols(Elf_Shdr *relasec, struct module *pmod)
relas = (Elf_Rela *) relasec->sh_addr;
/* For each rela in this klp relocation section */
for (i = 0; i < relasec->sh_size / sizeof(Elf_Rela); i++) {
- sym = pmod->core_kallsyms.symtab + ELF_R_SYM(relas[i].r_info);
+ sym = (Elf64_Sym *)sechdrs[symndx].sh_addr + ELF_R_SYM(relas[i].r_info);
if (sym->st_shndx != SHN_LIVEPATCH) {
pr_err("symbol %s is not marked as a livepatch symbol\n",
strtab + sym->st_name);
@@ -246,23 +246,41 @@ static int klp_resolve_symbols(Elf_Shdr *relasec, struct module *pmod)
return 0;
}
-static int klp_write_object_relocations(struct module *pmod,
- struct klp_object *obj)
+/*
+ * At a high-level, there are two types of klp relocation sections: those which
+ * reference symbols which live in vmlinux; and those which reference symbols
+ * which live in other modules. This function is called for both types:
+ *
+ * 1) When a klp module itself loads, the module code calls this function to
+ * write vmlinux-specific klp relocations (.klp.rela.vmlinux.* sections).
+ * These relocations are written to the klp module text to allow the patched
+ * code/data to reference unexported vmlinux symbols. They're written as
+ * early as possible to ensure that other module init code (.e.g.,
+ * jump_label_apply_nops) can access any unexported vmlinux symbols which
+ * might be referenced by the klp module's special sections.
+ *
+ * 2) When a to-be-patched module loads -- or is already loaded when a
+ * corresponding klp module loads -- klp code calls this function to write
+ * module-specific klp relocations (.klp.rela.{module}.* sections). These
+ * are written to the klp module text to allow the patched code/data to
+ * reference symbols which live in the to-be-patched module or one of its
+ * module dependencies. Exported symbols are supported, in addition to
+ * unexported symbols, in order to enable late module patching, which allows
+ * the to-be-patched module to be loaded and patched sometime *after* the
+ * klp module is loaded.
+ */
+int klp_write_relocations(Elf_Ehdr *ehdr, Elf_Shdr *sechdrs,
+ const char *shstrtab, const char *strtab,
+ unsigned int symndx, struct module *pmod,
+ const char *objname)
{
int i, cnt, ret = 0;
- const char *objname, *secname;
char sec_objname[MODULE_NAME_LEN];
Elf_Shdr *sec;
- if (WARN_ON(!klp_is_object_loaded(obj)))
- return -EINVAL;
-
- objname = klp_is_module(obj) ? obj->name : "vmlinux";
-
/* For each klp relocation section */
- for (i = 1; i < pmod->klp_info->hdr.e_shnum; i++) {
- sec = pmod->klp_info->sechdrs + i;
- secname = pmod->klp_info->secstrings + sec->sh_name;
+ for (i = 1; i < ehdr->e_shnum; i++) {
+ sec = sechdrs + i;
if (!(sec->sh_flags & SHF_RELA_LIVEPATCH))
continue;
@@ -271,24 +289,23 @@ static int klp_write_object_relocations(struct module *pmod,
* See comment in klp_resolve_symbols() for an explanation
* of the selected field width value.
*/
- cnt = sscanf(secname, ".klp.rela.%55[^.]", sec_objname);
+ cnt = sscanf(shstrtab + sec->sh_name, ".klp.rela.%55[^.]",
+ sec_objname);
if (cnt != 1) {
pr_err("section %s has an incorrectly formatted name\n",
- secname);
+ shstrtab + sec->sh_name);
ret = -EINVAL;
break;
}
- if (strcmp(objname, sec_objname))
+ if (strcmp(objname ? objname : "vmlinux", sec_objname))
continue;
- ret = klp_resolve_symbols(sec, pmod);
+ ret = klp_resolve_symbols(sechdrs, strtab, symndx, sec);
if (ret)
break;
- ret = apply_relocate_add(pmod->klp_info->sechdrs,
- pmod->core_kallsyms.strtab,
- pmod->klp_info->symndx, i, pmod);
+ ret = apply_relocate_add(sechdrs, strtab, symndx, i, pmod);
if (ret)
break;
}
@@ -736,20 +753,33 @@ static int klp_init_object_loaded(struct klp_patch *patch,
{
struct klp_func *func;
int ret;
+ struct klp_modinfo *info = patch->mod->klp_info;
mutex_lock(&text_mutex);
-
module_disable_ro(patch->mod);
- ret = klp_write_object_relocations(patch->mod, obj);
- if (ret) {
- module_enable_ro(patch->mod, true);
- mutex_unlock(&text_mutex);
- return ret;
+
+ if (klp_is_module(obj)) {
+ /*
+ * Only write module-specific relocations here
+ * (.klp.rela.{module}.*). vmlinux-specific relocations were
+ * written earlier during the initialization of the klp module
+ * itself.
+ */
+ ret = klp_write_relocations(&info->hdr, info->sechdrs,
+ info->secstrings,
+ patch->mod->core_kallsyms.strtab,
+ info->symndx, patch->mod,
+ obj->name);
+ if (ret) {
+ module_enable_ro(patch->mod, true);
+ mutex_unlock(&text_mutex);
+ return ret;
+ }
}
arch_klp_init_object_loaded(patch, obj);
- module_enable_ro(patch->mod, true);
+ module_enable_ro(patch->mod, true);
mutex_unlock(&text_mutex);
klp_for_each_func(obj, func) {
diff --git a/kernel/module.c b/kernel/module.c
index 646f1e2330d2..d36ea8a8c3ec 100644
--- a/kernel/module.c
+++ b/kernel/module.c
@@ -2334,11 +2334,12 @@ static int apply_relocations(struct module *mod, const struct load_info *info)
if (!(info->sechdrs[infosec].sh_flags & SHF_ALLOC))
continue;
- /* Livepatch relocation sections are applied by livepatch */
if (info->sechdrs[i].sh_flags & SHF_RELA_LIVEPATCH)
- continue;
-
- if (info->sechdrs[i].sh_type == SHT_REL)
+ err = klp_write_relocations(info->hdr, info->sechdrs,
+ info->secstrings,
+ info->strtab,
+ info->index.sym, mod, NULL);
+ else if (info->sechdrs[i].sh_type == SHT_REL)
err = apply_relocate(info->sechdrs, info->strtab,
info->index.sym, i, mod);
else if (info->sechdrs[i].sh_type == SHT_RELA)
--
2.21.1
On Fri, Apr 17, 2020 at 09:04:27AM -0500, Josh Poimboeuf wrote:
>
> [ ... snip ... ]
>
> diff --git a/kernel/livepatch/core.c b/kernel/livepatch/core.c
> index 40cfac8156fd..5fda3afc0285 100644
> --- a/kernel/livepatch/core.c
> +++ b/kernel/livepatch/core.c
>
> [ ... snip ... ]
>
> +int klp_write_relocations(Elf_Ehdr *ehdr, Elf_Shdr *sechdrs,
> + const char *shstrtab, const char *strtab,
> + unsigned int symndx, struct module *pmod,
> + const char *objname)
> {
> int i, cnt, ret = 0;
> - const char *objname, *secname;
> char sec_objname[MODULE_NAME_LEN];
> Elf_Shdr *sec;
>
> - if (WARN_ON(!klp_is_object_loaded(obj)))
> - return -EINVAL;
> -
> - objname = klp_is_module(obj) ? obj->name : "vmlinux";
> -
> /* For each klp relocation section */
> - for (i = 1; i < pmod->klp_info->hdr.e_shnum; i++) {
> - sec = pmod->klp_info->sechdrs + i;
> - secname = pmod->klp_info->secstrings + sec->sh_name;
> + for (i = 1; i < ehdr->e_shnum; i++) {
> + sec = sechdrs + i;
Hi Josh, minor bug:
Note the for loop through the section headers in
klp_write_relocations(), but its calling function ...
> [ ... snip ... ]
>
> diff --git a/kernel/module.c b/kernel/module.c
> index 646f1e2330d2..d36ea8a8c3ec 100644
> --- a/kernel/module.c
> +++ b/kernel/module.c
> @@ -2334,11 +2334,12 @@ static int apply_relocations(struct module *mod, const struct load_info *info)
> if (!(info->sechdrs[infosec].sh_flags & SHF_ALLOC))
> continue;
>
> - /* Livepatch relocation sections are applied by livepatch */
> if (info->sechdrs[i].sh_flags & SHF_RELA_LIVEPATCH)
> - continue;
> -
> - if (info->sechdrs[i].sh_type == SHT_REL)
> + err = klp_write_relocations(info->hdr, info->sechdrs,
> + info->secstrings,
> + info->strtab,
> + info->index.sym, mod, NULL);
> + else if (info->sechdrs[i].sh_type == SHT_REL)
> err = apply_relocate(info->sechdrs, info->strtab,
> info->index.sym, i, mod);
> else if (info->sechdrs[i].sh_type == SHT_RELA)
... apply_relocations() is also iterating over the section headers (the
diff context doesn't show it here, but i is an incrementing index over
sechdrs[]).
So if there is more than one KLP relocation section, we'll process them
multiple times. At least the x86 relocation code will detect this and
fail the module load with an invalid relocation (existing value not
zero).
-- Joe
On Mon, Apr 20, 2020 at 01:57:51PM -0400, Joe Lawrence wrote:
> On Fri, Apr 17, 2020 at 09:04:27AM -0500, Josh Poimboeuf wrote:
> >
> > [ ... snip ... ]
> >
> > diff --git a/kernel/livepatch/core.c b/kernel/livepatch/core.c
> > index 40cfac8156fd..5fda3afc0285 100644
> > --- a/kernel/livepatch/core.c
> > +++ b/kernel/livepatch/core.c
> >
> > [ ... snip ... ]
> >
> > +int klp_write_relocations(Elf_Ehdr *ehdr, Elf_Shdr *sechdrs,
> > + const char *shstrtab, const char *strtab,
> > + unsigned int symndx, struct module *pmod,
> > + const char *objname)
> > {
> > int i, cnt, ret = 0;
> > - const char *objname, *secname;
> > char sec_objname[MODULE_NAME_LEN];
> > Elf_Shdr *sec;
> >
> > - if (WARN_ON(!klp_is_object_loaded(obj)))
> > - return -EINVAL;
> > -
> > - objname = klp_is_module(obj) ? obj->name : "vmlinux";
> > -
> > /* For each klp relocation section */
> > - for (i = 1; i < pmod->klp_info->hdr.e_shnum; i++) {
> > - sec = pmod->klp_info->sechdrs + i;
> > - secname = pmod->klp_info->secstrings + sec->sh_name;
> > + for (i = 1; i < ehdr->e_shnum; i++) {
> > + sec = sechdrs + i;
>
> Hi Josh, minor bug:
>
> Note the for loop through the section headers in
> klp_write_relocations(), but its calling function ...
>
> > [ ... snip ... ]
> >
> > diff --git a/kernel/module.c b/kernel/module.c
> > index 646f1e2330d2..d36ea8a8c3ec 100644
> > --- a/kernel/module.c
> > +++ b/kernel/module.c
> > @@ -2334,11 +2334,12 @@ static int apply_relocations(struct module *mod, const struct load_info *info)
> > if (!(info->sechdrs[infosec].sh_flags & SHF_ALLOC))
> > continue;
> >
> > - /* Livepatch relocation sections are applied by livepatch */
> > if (info->sechdrs[i].sh_flags & SHF_RELA_LIVEPATCH)
> > - continue;
> > -
> > - if (info->sechdrs[i].sh_type == SHT_REL)
> > + err = klp_write_relocations(info->hdr, info->sechdrs,
> > + info->secstrings,
> > + info->strtab,
> > + info->index.sym, mod, NULL);
> > + else if (info->sechdrs[i].sh_type == SHT_REL)
> > err = apply_relocate(info->sechdrs, info->strtab,
> > info->index.sym, i, mod);
> > else if (info->sechdrs[i].sh_type == SHT_RELA)
>
> ... apply_relocations() is also iterating over the section headers (the
> diff context doesn't show it here, but i is an incrementing index over
> sechdrs[]).
>
> So if there is more than one KLP relocation section, we'll process them
> multiple times. At least the x86 relocation code will detect this and
> fail the module load with an invalid relocation (existing value not
> zero).
Ah, yes, good catch!
--
Josh
On Mon, Apr 20, 2020 at 01:25:16PM -0500, Josh Poimboeuf wrote:
> On Mon, Apr 20, 2020 at 01:57:51PM -0400, Joe Lawrence wrote:
> > On Fri, Apr 17, 2020 at 09:04:27AM -0500, Josh Poimboeuf wrote:
> > >
> > > [ ... snip ... ]
> > >
> > > diff --git a/kernel/livepatch/core.c b/kernel/livepatch/core.c
> > > index 40cfac8156fd..5fda3afc0285 100644
> > > --- a/kernel/livepatch/core.c
> > > +++ b/kernel/livepatch/core.c
> > >
> > > [ ... snip ... ]
> > >
> > > +int klp_write_relocations(Elf_Ehdr *ehdr, Elf_Shdr *sechdrs,
> > > + const char *shstrtab, const char *strtab,
> > > + unsigned int symndx, struct module *pmod,
> > > + const char *objname)
> > > {
> > > int i, cnt, ret = 0;
> > > - const char *objname, *secname;
> > > char sec_objname[MODULE_NAME_LEN];
> > > Elf_Shdr *sec;
> > >
> > > - if (WARN_ON(!klp_is_object_loaded(obj)))
> > > - return -EINVAL;
> > > -
> > > - objname = klp_is_module(obj) ? obj->name : "vmlinux";
> > > -
> > > /* For each klp relocation section */
> > > - for (i = 1; i < pmod->klp_info->hdr.e_shnum; i++) {
> > > - sec = pmod->klp_info->sechdrs + i;
> > > - secname = pmod->klp_info->secstrings + sec->sh_name;
> > > + for (i = 1; i < ehdr->e_shnum; i++) {
> > > + sec = sechdrs + i;
> >
> > Hi Josh, minor bug:
> >
> > Note the for loop through the section headers in
> > klp_write_relocations(), but its calling function ...
> >
> > > [ ... snip ... ]
> > >
> > > diff --git a/kernel/module.c b/kernel/module.c
> > > index 646f1e2330d2..d36ea8a8c3ec 100644
> > > --- a/kernel/module.c
> > > +++ b/kernel/module.c
> > > @@ -2334,11 +2334,12 @@ static int apply_relocations(struct module *mod, const struct load_info *info)
> > > if (!(info->sechdrs[infosec].sh_flags & SHF_ALLOC))
> > > continue;
> > >
> > > - /* Livepatch relocation sections are applied by livepatch */
> > > if (info->sechdrs[i].sh_flags & SHF_RELA_LIVEPATCH)
> > > - continue;
> > > -
> > > - if (info->sechdrs[i].sh_type == SHT_REL)
> > > + err = klp_write_relocations(info->hdr, info->sechdrs,
> > > + info->secstrings,
> > > + info->strtab,
> > > + info->index.sym, mod, NULL);
> > > + else if (info->sechdrs[i].sh_type == SHT_REL)
> > > err = apply_relocate(info->sechdrs, info->strtab,
> > > info->index.sym, i, mod);
> > > else if (info->sechdrs[i].sh_type == SHT_RELA)
> >
> > ... apply_relocations() is also iterating over the section headers (the
> > diff context doesn't show it here, but i is an incrementing index over
> > sechdrs[]).
> >
> > So if there is more than one KLP relocation section, we'll process them
> > multiple times. At least the x86 relocation code will detect this and
> > fail the module load with an invalid relocation (existing value not
> > zero).
>
> Ah, yes, good catch!
>
The same test case passed with a small modification to push the foreach
KLP section part to a kernel/livepatch/core.c local function and
exposing the klp_resolve_symbols() + apply_relocate_add() for a given
section to kernel/module.c. Something like following...
-- Joe
-->8-- -->8-- -->8-- -->8-- -->8-- -->8-- -->8-- -->8-- -->8-- -->8--
diff --git a/include/linux/livepatch.h b/include/linux/livepatch.h
index 8b6886c2d5c7..516f285ccc82 100644
--- a/include/linux/livepatch.h
+++ b/include/linux/livepatch.h
@@ -232,9 +232,9 @@ void klp_shadow_free_all(unsigned long id, klp_shadow_dtor_t dtor);
struct klp_state *klp_get_state(struct klp_patch *patch, unsigned long id);
struct klp_state *klp_get_prev_state(unsigned long id);
-int klp_write_relocations(Elf_Ehdr *ehdr, Elf_Shdr *sechdrs,
- const char *shstrtab, const char *strtab,
- unsigned int symindex, struct module *pmod,
+int klp_write_relocations(Elf_Shdr *sechdrs, const char *shstrtab,
+ const char *strtab, unsigned int symndx,
+ unsigned int relsec, struct module *pmod,
const char *objname);
/* Used to annotate symbol relocations in livepatches */
diff --git a/kernel/livepatch/core.c b/kernel/livepatch/core.c
index d2610f63e70b..d74fd7d10f16 100644
--- a/kernel/livepatch/core.c
+++ b/kernel/livepatch/core.c
@@ -285,14 +285,48 @@ static int klp_resolve_symbols(Elf64_Shdr *sechdrs, const char *strtab,
* the to-be-patched module to be loaded and patched sometime *after* the
* klp module is loaded.
*/
-int klp_write_relocations(Elf_Ehdr *ehdr, Elf_Shdr *sechdrs,
- const char *shstrtab, const char *strtab,
- unsigned int symndx, struct module *pmod,
+
+int klp_write_relocations(Elf_Shdr *sechdrs, const char *shstrtab,
+ const char *strtab, unsigned int symndx,
+ unsigned int relsec, struct module *pmod,
const char *objname)
{
- int i, cnt, ret = 0;
char sec_objname[MODULE_NAME_LEN];
Elf_Shdr *sec;
+ int cnt, ret;
+
+ sec = sechdrs + relsec;
+
+ /*
+ * Format: .klp.rela.sec_objname.section_name
+ * See comment in klp_resolve_symbols() for an explanation
+ * of the selected field width value.
+ */
+ cnt = sscanf(shstrtab + sec->sh_name, KLP_RELA_PREFIX "%55[^.]",
+ sec_objname);
+ if (cnt != 1) {
+ pr_err("section %s has an incorrectly formatted name\n",
+ shstrtab + sec->sh_name);
+ return -EINVAL;
+ }
+
+ if (strcmp(objname ? objname : "vmlinux", sec_objname))
+ return 0;
+
+ ret = klp_resolve_symbols(sechdrs, strtab, symndx, sec, sec_objname);
+ if (ret)
+ return ret;
+
+ return apply_relocate_add(sechdrs, strtab, symndx, relsec, pmod);
+}
+
+static int klp_write_all_relocations(Elf_Ehdr *ehdr, Elf_Shdr *sechdrs,
+ const char *shstrtab, const char *strtab,
+ unsigned int symndx, struct module *pmod,
+ const char *objname)
+{
+ int i, ret;
+ Elf_Shdr *sec;
/* For each klp relocation section */
for (i = 1; i < ehdr->e_shnum; i++) {
@@ -300,34 +334,13 @@ int klp_write_relocations(Elf_Ehdr *ehdr, Elf_Shdr *sechdrs,
if (!(sec->sh_flags & SHF_RELA_LIVEPATCH))
continue;
- /*
- * Format: .klp.rela.sec_objname.section_name
- * See comment in klp_resolve_symbols() for an explanation
- * of the selected field width value.
- */
- cnt = sscanf(shstrtab + sec->sh_name, KLP_RELA_PREFIX "%55[^.]",
- sec_objname);
- if (cnt != 1) {
- pr_err("section %s has an incorrectly formatted name\n",
- shstrtab + sec->sh_name);
- ret = -EINVAL;
- break;
- }
-
- if (strcmp(objname ? objname : "vmlinux", sec_objname))
- continue;
-
- ret = klp_resolve_symbols(sechdrs, strtab, symndx, sec,
- sec_objname);
+ ret = klp_write_relocations(sechdrs, shstrtab, strtab, symndx,
+ i, pmod, objname);
if (ret)
- break;
-
- ret = apply_relocate_add(sechdrs, strtab, symndx, i, pmod);
- if (ret)
- break;
+ return ret;
}
- return ret;
+ return 0;
}
/*
@@ -773,11 +786,11 @@ static int klp_init_object_loaded(struct klp_patch *patch,
* written earlier during the initialization of the klp module
* itself.
*/
- ret = klp_write_relocations(&info->hdr, info->sechdrs,
- info->secstrings,
- patch->mod->core_kallsyms.strtab,
- info->symndx, patch->mod,
- obj->name);
+ ret = klp_write_all_relocations(&info->hdr, info->sechdrs,
+ info->secstrings,
+ patch->mod->core_kallsyms.strtab,
+ info->symndx, patch->mod,
+ obj->name);
if (ret)
return ret;
}
diff --git a/kernel/module.c b/kernel/module.c
index 86736e2ff73d..04e5f5d55eb4 100644
--- a/kernel/module.c
+++ b/kernel/module.c
@@ -2322,10 +2322,11 @@ static int apply_relocations(struct module *mod, const struct load_info *info)
continue;
if (info->sechdrs[i].sh_flags & SHF_RELA_LIVEPATCH)
- err = klp_write_relocations(info->hdr, info->sechdrs,
+ err = klp_write_relocations(info->sechdrs,
info->secstrings,
info->strtab,
- info->index.sym, mod, NULL);
+ info->index.sym, i, mod,
+ NULL);
else if (info->sechdrs[i].sh_type == SHT_REL)
err = apply_relocate(info->sechdrs, info->strtab,
info->index.sym, i, mod);
On Mon, Apr 20, 2020 at 03:01:41PM -0400, Joe Lawrence wrote:
> > > ... apply_relocations() is also iterating over the section headers (the
> > > diff context doesn't show it here, but i is an incrementing index over
> > > sechdrs[]).
> > >
> > > So if there is more than one KLP relocation section, we'll process them
> > > multiple times. At least the x86 relocation code will detect this and
> > > fail the module load with an invalid relocation (existing value not
> > > zero).
> >
> > Ah, yes, good catch!
> >
>
> The same test case passed with a small modification to push the foreach
> KLP section part to a kernel/livepatch/core.c local function and
> exposing the klp_resolve_symbols() + apply_relocate_add() for a given
> section to kernel/module.c. Something like following...
I came up with something very similar, though I named them
klp_apply_object_relocs() and klp_apply_section_relocs() and changed the
argument order a bit (module first). Since it sounds like you have a
test, could you try this one?
diff --git a/include/linux/livepatch.h b/include/linux/livepatch.h
index 533359e48c39..fb1a3de39726 100644
--- a/include/linux/livepatch.h
+++ b/include/linux/livepatch.h
@@ -231,10 +231,10 @@ void klp_shadow_free_all(unsigned long id, klp_shadow_dtor_t dtor);
struct klp_state *klp_get_state(struct klp_patch *patch, unsigned long id);
struct klp_state *klp_get_prev_state(unsigned long id);
-int klp_write_relocations(Elf_Ehdr *ehdr, Elf_Shdr *sechdrs,
- const char *shstrtab, const char *strtab,
- unsigned int symindex, struct module *pmod,
- const char *objname);
+int klp_apply_section_relocs(struct module *pmod, Elf_Shdr *sechdrs,
+ const char *shstrtab, const char *strtab,
+ unsigned int symindex, unsigned int secindex,
+ const char *objname);
#else /* !CONFIG_LIVEPATCH */
@@ -245,10 +245,10 @@ static inline void klp_update_patch_state(struct task_struct *task) {}
static inline void klp_copy_process(struct task_struct *child) {}
static inline
-int klp_write_relocations(Elf_Ehdr *ehdr, Elf_Shdr *sechdrs,
- const char *shstrtab, const char *strtab,
- unsigned int symindex, struct module *pmod,
- const char *objname)
+int klp_apply_section_relocs(struct module *pmod, Elf_Shdr *sechdrs,
+ const char *shstrtab, const char *strtab,
+ unsigned int symindex, unsigned int secindex,
+ const char *objname);
{
return 0;
}
diff --git a/kernel/livepatch/core.c b/kernel/livepatch/core.c
index 3ff886b911ae..89c5cb962c54 100644
--- a/kernel/livepatch/core.c
+++ b/kernel/livepatch/core.c
@@ -285,49 +285,37 @@ static int klp_resolve_symbols(Elf64_Shdr *sechdrs, const char *strtab,
* the to-be-patched module to be loaded and patched sometime *after* the
* klp module is loaded.
*/
-int klp_write_relocations(Elf_Ehdr *ehdr, Elf_Shdr *sechdrs,
- const char *shstrtab, const char *strtab,
- unsigned int symndx, struct module *pmod,
- const char *objname)
+int klp_apply_section_relocs(struct module *pmod, Elf_Shdr *sechdrs,
+ const char *shstrtab, const char *strtab,
+ unsigned int symndx, unsigned int secndx,
+ const char *objname)
{
- int i, cnt, ret = 0;
+ int cnt, ret;
char sec_objname[MODULE_NAME_LEN];
- Elf_Shdr *sec;
+ Elf_Shdr *sec = sechdrs + secndx;
- /* For each klp relocation section */
- for (i = 1; i < ehdr->e_shnum; i++) {
- sec = sechdrs + i;
- if (!(sec->sh_flags & SHF_RELA_LIVEPATCH))
- continue;
-
- /*
- * Format: .klp.rela.sec_objname.section_name
- * See comment in klp_resolve_symbols() for an explanation
- * of the selected field width value.
- */
- cnt = sscanf(shstrtab + sec->sh_name, ".klp.rela.%55[^.]",
- sec_objname);
- if (cnt != 1) {
- pr_err("section %s has an incorrectly formatted name\n",
- shstrtab + sec->sh_name);
- ret = -EINVAL;
- break;
- }
-
- if (strcmp(objname ? objname : "vmlinux", sec_objname))
- continue;
+ /*
+ * Format: .klp.rela.sec_objname.section_name
+ * See comment in klp_resolve_symbols() for an explanation
+ * of the selected field width value.
+ */
+ cnt = sscanf(shstrtab + sec->sh_name, ".klp.rela.%55[^.]",
+ sec_objname);
+ if (cnt != 1) {
+ pr_err("section %s has an incorrectly formatted name\n",
+ shstrtab + sec->sh_name);
+ return -EINVAL;
+ }
- ret = klp_resolve_symbols(sechdrs, strtab, symndx, sec,
- sec_objname);
- if (ret)
- break;
+ if (strcmp(objname ? objname : "vmlinux", sec_objname))
+ return 0;
- ret = apply_relocate_add(sechdrs, strtab, symndx, i, pmod);
- if (ret)
- break;
- }
+ ret = klp_resolve_symbols(sechdrs, strtab, symndx, sec,
+ sec_objname);
+ if (ret)
+ return ret;
- return ret;
+ return apply_relocate_add(sechdrs, strtab, symndx, secndx, pmod);
}
/*
@@ -758,13 +746,34 @@ static int klp_init_func(struct klp_object *obj, struct klp_func *func)
func->old_sympos ? func->old_sympos : 1);
}
+int klp_apply_object_relocs(struct klp_patch *patch, struct klp_object *obj)
+{
+ int i, ret;
+ struct klp_modinfo *info = patch->mod->klp_info;
+
+ for (i = 1; i < info->hdr.e_shnum; i++) {
+ Elf_Shdr *sec = info->sechdrs + i;
+
+ if (!(sec->sh_flags & SHF_RELA_LIVEPATCH))
+ continue;
+
+ ret = klp_apply_section_relocs(patch->mod, info->sechdrs,
+ info->secstrings,
+ patch->mod->core_kallsyms.strtab,
+ info->symndx, i, obj->name);
+ if (ret)
+ return ret;
+ }
+
+ return 0;
+}
+
/* parts of the initialization that is done only when the object is loaded */
static int klp_init_object_loaded(struct klp_patch *patch,
struct klp_object *obj)
{
struct klp_func *func;
int ret;
- struct klp_modinfo *info = patch->mod->klp_info;
if (klp_is_module(obj)) {
/*
@@ -773,11 +782,7 @@ static int klp_init_object_loaded(struct klp_patch *patch,
* written earlier during the initialization of the klp module
* itself.
*/
- ret = klp_write_relocations(&info->hdr, info->sechdrs,
- info->secstrings,
- patch->mod->core_kallsyms.strtab,
- info->symndx, patch->mod,
- obj->name);
+ ret = klp_apply_object_relocs(patch, obj);
if (ret)
return ret;
}
diff --git a/kernel/module.c b/kernel/module.c
index b1d30ad67e82..3ba024afe379 100644
--- a/kernel/module.c
+++ b/kernel/module.c
@@ -2322,10 +2322,11 @@ static int apply_relocations(struct module *mod, const struct load_info *info)
continue;
if (info->sechdrs[i].sh_flags & SHF_RELA_LIVEPATCH)
- err = klp_write_relocations(info->hdr, info->sechdrs,
- info->secstrings,
- info->strtab,
- info->index.sym, mod, NULL);
+ err = klp_apply_section_relocs(mod, info->sechdrs,
+ info->secstrings,
+ info->strtab,
+ info->index.sym, i,
+ NULL);
else if (info->sechdrs[i].sh_type == SHT_REL)
err = apply_relocate(info->sechdrs, info->strtab,
info->index.sym, i, mod);
On Mon, Apr 20, 2020 at 02:11:17PM -0500, Josh Poimboeuf wrote:
> On Mon, Apr 20, 2020 at 03:01:41PM -0400, Joe Lawrence wrote:
> > > > ... apply_relocations() is also iterating over the section headers (the
> > > > diff context doesn't show it here, but i is an incrementing index over
> > > > sechdrs[]).
> > > >
> > > > So if there is more than one KLP relocation section, we'll process them
> > > > multiple times. At least the x86 relocation code will detect this and
> > > > fail the module load with an invalid relocation (existing value not
> > > > zero).
> > >
> > > Ah, yes, good catch!
> > >
> >
> > The same test case passed with a small modification to push the foreach
> > KLP section part to a kernel/livepatch/core.c local function and
> > exposing the klp_resolve_symbols() + apply_relocate_add() for a given
> > section to kernel/module.c. Something like following...
>
> I came up with something very similar, though I named them
> klp_apply_object_relocs() and klp_apply_section_relocs() and changed the
> argument order a bit (module first). Since it sounds like you have a
> test, could you try this one?
>
LGTM. I have a few klp-convert selftests that I've been slowly
tinkering on and they all load/run successfully with this version. :)
-- Joe
On Mon, Apr 20, 2020 at 03:49:00PM -0400, Joe Lawrence wrote:
> On Mon, Apr 20, 2020 at 02:11:17PM -0500, Josh Poimboeuf wrote:
> > On Mon, Apr 20, 2020 at 03:01:41PM -0400, Joe Lawrence wrote:
> > > > > ... apply_relocations() is also iterating over the section headers (the
> > > > > diff context doesn't show it here, but i is an incrementing index over
> > > > > sechdrs[]).
> > > > >
> > > > > So if there is more than one KLP relocation section, we'll process them
> > > > > multiple times. At least the x86 relocation code will detect this and
> > > > > fail the module load with an invalid relocation (existing value not
> > > > > zero).
> > > >
> > > > Ah, yes, good catch!
> > > >
> > >
> > > The same test case passed with a small modification to push the foreach
> > > KLP section part to a kernel/livepatch/core.c local function and
> > > exposing the klp_resolve_symbols() + apply_relocate_add() for a given
> > > section to kernel/module.c. Something like following...
> >
> > I came up with something very similar, though I named them
> > klp_apply_object_relocs() and klp_apply_section_relocs() and changed the
> > argument order a bit (module first). Since it sounds like you have a
> > test, could you try this one?
> >
>
> LGTM. I have a few klp-convert selftests that I've been slowly
> tinkering on and they all load/run successfully with this version. :)
Good to hear, thanks! Hooray selftests :-)
--
Josh
On Mon, 20 Apr 2020, Joe Lawrence wrote:
> On Fri, Apr 17, 2020 at 09:04:27AM -0500, Josh Poimboeuf wrote:
> >
> > [ ... snip ... ]
> >
> > diff --git a/kernel/livepatch/core.c b/kernel/livepatch/core.c
> > index 40cfac8156fd..5fda3afc0285 100644
> > --- a/kernel/livepatch/core.c
> > +++ b/kernel/livepatch/core.c
> >
> > [ ... snip ... ]
> >
> > +int klp_write_relocations(Elf_Ehdr *ehdr, Elf_Shdr *sechdrs,
> > + const char *shstrtab, const char *strtab,
> > + unsigned int symndx, struct module *pmod,
> > + const char *objname)
> > {
> > int i, cnt, ret = 0;
> > - const char *objname, *secname;
> > char sec_objname[MODULE_NAME_LEN];
> > Elf_Shdr *sec;
> >
> > - if (WARN_ON(!klp_is_object_loaded(obj)))
> > - return -EINVAL;
> > -
> > - objname = klp_is_module(obj) ? obj->name : "vmlinux";
> > -
> > /* For each klp relocation section */
> > - for (i = 1; i < pmod->klp_info->hdr.e_shnum; i++) {
> > - sec = pmod->klp_info->sechdrs + i;
> > - secname = pmod->klp_info->secstrings + sec->sh_name;
> > + for (i = 1; i < ehdr->e_shnum; i++) {
> > + sec = sechdrs + i;
>
> Hi Josh, minor bug:
>
> Note the for loop through the section headers in
> klp_write_relocations(), but its calling function ...
>
> > [ ... snip ... ]
> >
> > diff --git a/kernel/module.c b/kernel/module.c
> > index 646f1e2330d2..d36ea8a8c3ec 100644
> > --- a/kernel/module.c
> > +++ b/kernel/module.c
> > @@ -2334,11 +2334,12 @@ static int apply_relocations(struct module *mod, const struct load_info *info)
> > if (!(info->sechdrs[infosec].sh_flags & SHF_ALLOC))
> > continue;
> >
> > - /* Livepatch relocation sections are applied by livepatch */
> > if (info->sechdrs[i].sh_flags & SHF_RELA_LIVEPATCH)
> > - continue;
> > -
> > - if (info->sechdrs[i].sh_type == SHT_REL)
> > + err = klp_write_relocations(info->hdr, info->sechdrs,
> > + info->secstrings,
> > + info->strtab,
> > + info->index.sym, mod, NULL);
> > + else if (info->sechdrs[i].sh_type == SHT_REL)
> > err = apply_relocate(info->sechdrs, info->strtab,
> > info->index.sym, i, mod);
> > else if (info->sechdrs[i].sh_type == SHT_RELA)
>
> ... apply_relocations() is also iterating over the section headers (the
> diff context doesn't show it here, but i is an incrementing index over
> sechdrs[]).
>
> So if there is more than one KLP relocation section, we'll process them
> multiple times. At least the x86 relocation code will detect this and
> fail the module load with an invalid relocation (existing value not
> zero).
The last paragraph confused me a little. I'm sending the following, so it
is archived publicly.
If there is more than one KLP relocation section in a patch module, let's
say for vmlinux and some arbitrary module, klp_write_relocations() will
be called multiple times from apply_relocations(). Each time with NULL as
the last parameter, so each time vmlinux relocation section will be
processed and x86 relocation code will detect this the second time and
fail.
If there was no relocation section for vmlinux, but for multiple arbitrary
modules, all should be "fine", because klp_write_relocations() would just
skip everything.
Anyway, good catch, I missed it completely.
Miroslav
On Mon, Apr 20, 2020 at 02:11:17PM -0500, Josh Poimboeuf wrote:
> On Mon, Apr 20, 2020 at 03:01:41PM -0400, Joe Lawrence wrote:
> > > > ... apply_relocations() is also iterating over the section headers (the
> > > > diff context doesn't show it here, but i is an incrementing index over
> > > > sechdrs[]).
> > > >
> > > > So if there is more than one KLP relocation section, we'll process them
> > > > multiple times. At least the x86 relocation code will detect this and
> > > > fail the module load with an invalid relocation (existing value not
> > > > zero).
> > >
> > > Ah, yes, good catch!
> > >
> >
> > The same test case passed with a small modification to push the foreach
> > KLP section part to a kernel/livepatch/core.c local function and
> > exposing the klp_resolve_symbols() + apply_relocate_add() for a given
> > section to kernel/module.c. Something like following...
>
> I came up with something very similar, though I named them
> klp_apply_object_relocs() and klp_apply_section_relocs() and changed the
> argument order a bit (module first). Since it sounds like you have a
> test, could you try this one?
>
> diff --git a/include/linux/livepatch.h b/include/linux/livepatch.h
> index 533359e48c39..fb1a3de39726 100644
> --- a/include/linux/livepatch.h
> +++ b/include/linux/livepatch.h
>
> [ ... snip ... ]
>
> @@ -245,10 +245,10 @@ static inline void klp_update_patch_state(struct task_struct *task) {}
> static inline void klp_copy_process(struct task_struct *child) {}
>
> static inline
> -int klp_write_relocations(Elf_Ehdr *ehdr, Elf_Shdr *sechdrs,
> - const char *shstrtab, const char *strtab,
> - unsigned int symindex, struct module *pmod,
> - const char *objname)
> +int klp_apply_section_relocs(struct module *pmod, Elf_Shdr *sechdrs,
> + const char *shstrtab, const char *strtab,
> + unsigned int symindex, unsigned int secindex,
> + const char *objname);
^^
Whoops, stray semicolon in !CONFIG_LIVEPATCH case. I found it by
botching my cross-compiling .config, but the build-bot might find it
when you push your branch.
> {
> return 0;
> }
-- Joe