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]>
Signed-off-by: Josh Poimboeuf <[email protected]>
---
include/linux/livepatch.h | 16 ++++++++
kernel/livepatch/core.c | 86 ++++++++++++++++++++++++++-------------
kernel/module.c | 9 ++--
3 files changed, 79 insertions(+), 32 deletions(-)
diff --git a/include/linux/livepatch.h b/include/linux/livepatch.h
index e894e74905f3..d9e9b76f6054 100644
--- a/include/linux/livepatch.h
+++ b/include/linux/livepatch.h
@@ -234,14 +234,30 @@ 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 */
+struct klp_object;
+
static inline int klp_module_coming(struct module *mod) { return 0; }
static inline void klp_module_going(struct module *mod) {}
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 c3512e7e0801..ac9e2e78ae0f 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 Tue, Apr 14, 2020 at 07:44:06PM +0200, Peter Zijlstra wrote:
> On Tue, Apr 14, 2020 at 11:28:37AM -0500, Josh Poimboeuf wrote:
> > KLP relocations are livepatch-specific relocations which are applied to
> > 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.
>
> Is there something that disallows a module from being called 'vmlinux' ?
> If not, we might want to enforce this somewhere.
I'm pretty sure we don't have a check for that anywhere, though the KLP
module would almost certainly fail during the module load when it
couldn't find the vmlinux.ko symbols it needed.
It wouldn't hurt to add a check somewhere though. Maybe in
klp_module_coming() since the restriction only applies to
CONFIG_LIVEPATCH...
--
Josh
On Tue, Apr 14, 2020 at 11:28:37AM -0500, Josh Poimboeuf wrote:
> KLP relocations are livepatch-specific relocations which are applied to
> 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.
Is there something that disallows a module from being called 'vmlinux' ?
If not, we might want to enforce this somewhere.
On Tue, Apr 14, 2020 at 01:01:09PM -0500, Josh Poimboeuf wrote:
> On Tue, Apr 14, 2020 at 07:44:06PM +0200, Peter Zijlstra wrote:
> > On Tue, Apr 14, 2020 at 11:28:37AM -0500, Josh Poimboeuf wrote:
> > > KLP relocations are livepatch-specific relocations which are applied to
> > > 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.
> >
> > Is there something that disallows a module from being called 'vmlinux' ?
> > If not, we might want to enforce this somewhere.
>
> I'm pretty sure we don't have a check for that anywhere, though the KLP
> module would almost certainly fail during the module load when it
> couldn't find the vmlinux.ko symbols it needed.
>
> It wouldn't hurt to add a check somewhere though. Maybe in
> klp_module_coming() since the restriction only applies to
> CONFIG_LIVEPATCH...
From: Josh Poimboeuf <[email protected]>
Subject: [PATCH] livepatch: Disallow vmlinux.ko
This is purely a theoretical issue, but if there were a module named
vmlinux.ko, the livepatch relocation code wouldn't be able to
distinguish between vmlinux-specific and vmlinux.o-specific KLP
relocations.
If CONFIG_LIVEPATCH is enabled, don't allow a module named vmlinux.ko.
Suggested-by: Peter Zijlstra <[email protected]>
Signed-off-by: Josh Poimboeuf <[email protected]>
---
kernel/livepatch/core.c | 5 +++++
1 file changed, 5 insertions(+)
diff --git a/kernel/livepatch/core.c b/kernel/livepatch/core.c
index 3a88639b3326..3ff886b911ae 100644
--- a/kernel/livepatch/core.c
+++ b/kernel/livepatch/core.c
@@ -1169,6 +1169,11 @@ int klp_module_coming(struct module *mod)
if (WARN_ON(mod->state != MODULE_STATE_COMING))
return -EINVAL;
+ if (!strcmp(mod->name, "vmlinux")) {
+ pr_err("vmlinux.ko: invalid module name");
+ return -EINVAL;
+ }
+
mutex_lock(&klp_mutex);
/*
* Each module has to know that klp_module_coming()
--
2.21.1
On Tue, 14 Apr 2020, Josh Poimboeuf wrote:
> On Tue, Apr 14, 2020 at 01:01:09PM -0500, Josh Poimboeuf wrote:
> > On Tue, Apr 14, 2020 at 07:44:06PM +0200, Peter Zijlstra wrote:
> > > On Tue, Apr 14, 2020 at 11:28:37AM -0500, Josh Poimboeuf wrote:
> > > > KLP relocations are livepatch-specific relocations which are applied to
> > > > 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.
> > >
> > > Is there something that disallows a module from being called 'vmlinux' ?
> > > If not, we might want to enforce this somewhere.
> >
> > I'm pretty sure we don't have a check for that anywhere, though the KLP
> > module would almost certainly fail during the module load when it
> > couldn't find the vmlinux.ko symbols it needed.
> >
> > It wouldn't hurt to add a check somewhere though. Maybe in
> > klp_module_coming() since the restriction only applies to
> > CONFIG_LIVEPATCH...
>
> From: Josh Poimboeuf <[email protected]>
> Subject: [PATCH] livepatch: Disallow vmlinux.ko
>
> This is purely a theoretical issue, but if there were a module named
OT: "if there were"... subjunctive?
> vmlinux.ko, the livepatch relocation code wouldn't be able to
> distinguish between vmlinux-specific and vmlinux.o-specific KLP
> relocations.
>
> If CONFIG_LIVEPATCH is enabled, don't allow a module named vmlinux.ko.
Yup, there is no such check nowadays. I always struggle to find the right
balance between being overprotective and letting the user shoot themselves
in their foot if they want to. But it does not hurt, so ack to that.
Miroslav
Just a nit below
> diff --git a/include/linux/livepatch.h b/include/linux/livepatch.h
> index e894e74905f3..d9e9b76f6054 100644
> --- a/include/linux/livepatch.h
> +++ b/include/linux/livepatch.h
> @@ -234,14 +234,30 @@ 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 */
>
> +struct klp_object;
> +
Is the forward declaration necessary here?
> static inline int klp_module_coming(struct module *mod) { return 0; }
> static inline void klp_module_going(struct module *mod) {}
> 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 */
Miroslav
On Wed, Apr 15, 2020 at 04:30:15PM +0200, Miroslav Benes wrote:
> > From: Josh Poimboeuf <[email protected]>
> > Subject: [PATCH] livepatch: Disallow vmlinux.ko
> >
> > This is purely a theoretical issue, but if there were a module named
>
> OT: "if there were"... subjunctive?
I had to google "subjunctive", but yes that seems to be it :-)
It means "if, hypothetically, a module named vmlinux.ko existed"...
> > vmlinux.ko, the livepatch relocation code wouldn't be able to
> > distinguish between vmlinux-specific and vmlinux.o-specific KLP
> > relocations.
> >
> > If CONFIG_LIVEPATCH is enabled, don't allow a module named vmlinux.ko.
>
> Yup, there is no such check nowadays. I always struggle to find the right
> balance between being overprotective and letting the user shoot themselves
> in their foot if they want to. But it does not hurt, so ack to that.
Yeah, and it does seem very unlikely to have a vmlinux.ko, but
especially OOT modules do some crazy things and you never know...
--
Josh
On Wed, Apr 15, 2020 at 04:34:26PM +0200, Miroslav Benes wrote:
> Just a nit below
>
> > diff --git a/include/linux/livepatch.h b/include/linux/livepatch.h
> > index e894e74905f3..d9e9b76f6054 100644
> > --- a/include/linux/livepatch.h
> > +++ b/include/linux/livepatch.h
> > @@ -234,14 +234,30 @@ 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 */
> >
> > +struct klp_object;
> > +
>
> Is the forward declaration necessary here?
Apparently not, that was leftover from a previous iteration...
> > static inline int klp_module_coming(struct module *mod) { return 0; }
> > static inline void klp_module_going(struct module *mod) {}
> > 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 */
>
> Miroslav
>
--
Josh