Re: [PATCH v2] arm64/module: use mod->klp_info section header information for livepatch modules
On 1 November 2018 at 17:07, Will Deacon wrote:
> Hello, Jessica,
>
> On Tue, Oct 30, 2018 at 02:19:10PM +0100, Jessica Yu wrote:
>> +++ Will Deacon [29/10/18 15:28 +]:
>> >On Fri, Oct 26, 2018 at 07:25:01PM +0200, Jessica Yu wrote:
>> >>diff --git a/arch/arm64/include/asm/module.h
>> >>b/arch/arm64/include/asm/module.h
>> >>index fef773c94e9d..ac9b97f9ae5e 100644
>> >>--- a/arch/arm64/include/asm/module.h
>> >>+++ b/arch/arm64/include/asm/module.h
>> >>@@ -25,6 +25,7 @@ struct mod_plt_sec {
>> >>struct elf64_shdr *plt;
>> >>int plt_num_entries;
>> >>int plt_max_entries;
>> >>+ int plt_shndx;
>> >>};
>> >
>> >Does this mean we can drop the plt pointer from this struct altogether, and
>> >simply offset into the section headers when applying the relocations?
>>
>> Hmm, if everyone is OK with dropping the plt pointer from struct
>> mod_plt_sec, then I think we can simplify this patch even further.
>>
>> With the plt shndx saved, we can additionally pass a pointer to
>> sechdrs to module_emit_plt_entry(), and with that just offset into the
>> section headers as you suggest. Since livepatch *already* passes in
>> the correct copy of the section headers (mod->klp_info->sechdrs) to
>> apply_relocate_add(), we wouldn't even need to modify the arm64
>> module_finalize() to change mod->arch.core.plt to point into
>> mod->klp_info->sechdrs anymore and we can drop all the changes to the
>> module loader too.
>>
>> Something like the following maybe?
>
> This looks pretty good, thanks! My only (minor) objection is that the
> renaming of plt_sec -> plt_info throughout makes the patch a lot more
> churny than it needs to be, for questionable gain.
>
> Anyway, it looks functionally correct and I've tested loading/unloading
> the "hello world" test module with both PLTs enabled and disabled.
>
> Acked-by: Will Deacon
>
For the simplified version:
Acked-by: Ard Biesheuvel
Re: [PATCH v2] arm64/module: use mod->klp_info section header information for livepatch modules
On 1 November 2018 at 17:07, Will Deacon wrote:
> Hello, Jessica,
>
> On Tue, Oct 30, 2018 at 02:19:10PM +0100, Jessica Yu wrote:
>> +++ Will Deacon [29/10/18 15:28 +]:
>> >On Fri, Oct 26, 2018 at 07:25:01PM +0200, Jessica Yu wrote:
>> >>diff --git a/arch/arm64/include/asm/module.h
>> >>b/arch/arm64/include/asm/module.h
>> >>index fef773c94e9d..ac9b97f9ae5e 100644
>> >>--- a/arch/arm64/include/asm/module.h
>> >>+++ b/arch/arm64/include/asm/module.h
>> >>@@ -25,6 +25,7 @@ struct mod_plt_sec {
>> >>struct elf64_shdr *plt;
>> >>int plt_num_entries;
>> >>int plt_max_entries;
>> >>+ int plt_shndx;
>> >>};
>> >
>> >Does this mean we can drop the plt pointer from this struct altogether, and
>> >simply offset into the section headers when applying the relocations?
>>
>> Hmm, if everyone is OK with dropping the plt pointer from struct
>> mod_plt_sec, then I think we can simplify this patch even further.
>>
>> With the plt shndx saved, we can additionally pass a pointer to
>> sechdrs to module_emit_plt_entry(), and with that just offset into the
>> section headers as you suggest. Since livepatch *already* passes in
>> the correct copy of the section headers (mod->klp_info->sechdrs) to
>> apply_relocate_add(), we wouldn't even need to modify the arm64
>> module_finalize() to change mod->arch.core.plt to point into
>> mod->klp_info->sechdrs anymore and we can drop all the changes to the
>> module loader too.
>>
>> Something like the following maybe?
>
> This looks pretty good, thanks! My only (minor) objection is that the
> renaming of plt_sec -> plt_info throughout makes the patch a lot more
> churny than it needs to be, for questionable gain.
>
> Anyway, it looks functionally correct and I've tested loading/unloading
> the "hello world" test module with both PLTs enabled and disabled.
>
> Acked-by: Will Deacon
>
For the simplified version:
Acked-by: Ard Biesheuvel
Re: [PATCH v2] arm64/module: use mod->klp_info section header information for livepatch modules
Hello, Jessica,
On Tue, Oct 30, 2018 at 02:19:10PM +0100, Jessica Yu wrote:
> +++ Will Deacon [29/10/18 15:28 +]:
> >On Fri, Oct 26, 2018 at 07:25:01PM +0200, Jessica Yu wrote:
> >>diff --git a/arch/arm64/include/asm/module.h
> >>b/arch/arm64/include/asm/module.h
> >>index fef773c94e9d..ac9b97f9ae5e 100644
> >>--- a/arch/arm64/include/asm/module.h
> >>+++ b/arch/arm64/include/asm/module.h
> >>@@ -25,6 +25,7 @@ struct mod_plt_sec {
> >>struct elf64_shdr *plt;
> >>int plt_num_entries;
> >>int plt_max_entries;
> >>+ int plt_shndx;
> >>};
> >
> >Does this mean we can drop the plt pointer from this struct altogether, and
> >simply offset into the section headers when applying the relocations?
>
> Hmm, if everyone is OK with dropping the plt pointer from struct
> mod_plt_sec, then I think we can simplify this patch even further.
>
> With the plt shndx saved, we can additionally pass a pointer to
> sechdrs to module_emit_plt_entry(), and with that just offset into the
> section headers as you suggest. Since livepatch *already* passes in
> the correct copy of the section headers (mod->klp_info->sechdrs) to
> apply_relocate_add(), we wouldn't even need to modify the arm64
> module_finalize() to change mod->arch.core.plt to point into
> mod->klp_info->sechdrs anymore and we can drop all the changes to the
> module loader too.
>
> Something like the following maybe?
This looks pretty good, thanks! My only (minor) objection is that the
renaming of plt_sec -> plt_info throughout makes the patch a lot more
churny than it needs to be, for questionable gain.
Anyway, it looks functionally correct and I've tested loading/unloading
the "hello world" test module with both PLTs enabled and disabled.
Acked-by: Will Deacon
Will
Re: [PATCH v2] arm64/module: use mod->klp_info section header information for livepatch modules
Hello, Jessica,
On Tue, Oct 30, 2018 at 02:19:10PM +0100, Jessica Yu wrote:
> +++ Will Deacon [29/10/18 15:28 +]:
> >On Fri, Oct 26, 2018 at 07:25:01PM +0200, Jessica Yu wrote:
> >>diff --git a/arch/arm64/include/asm/module.h
> >>b/arch/arm64/include/asm/module.h
> >>index fef773c94e9d..ac9b97f9ae5e 100644
> >>--- a/arch/arm64/include/asm/module.h
> >>+++ b/arch/arm64/include/asm/module.h
> >>@@ -25,6 +25,7 @@ struct mod_plt_sec {
> >>struct elf64_shdr *plt;
> >>int plt_num_entries;
> >>int plt_max_entries;
> >>+ int plt_shndx;
> >>};
> >
> >Does this mean we can drop the plt pointer from this struct altogether, and
> >simply offset into the section headers when applying the relocations?
>
> Hmm, if everyone is OK with dropping the plt pointer from struct
> mod_plt_sec, then I think we can simplify this patch even further.
>
> With the plt shndx saved, we can additionally pass a pointer to
> sechdrs to module_emit_plt_entry(), and with that just offset into the
> section headers as you suggest. Since livepatch *already* passes in
> the correct copy of the section headers (mod->klp_info->sechdrs) to
> apply_relocate_add(), we wouldn't even need to modify the arm64
> module_finalize() to change mod->arch.core.plt to point into
> mod->klp_info->sechdrs anymore and we can drop all the changes to the
> module loader too.
>
> Something like the following maybe?
This looks pretty good, thanks! My only (minor) objection is that the
renaming of plt_sec -> plt_info throughout makes the patch a lot more
churny than it needs to be, for questionable gain.
Anyway, it looks functionally correct and I've tested loading/unloading
the "hello world" test module with both PLTs enabled and disabled.
Acked-by: Will Deacon
Will
Re: [PATCH v2] arm64/module: use mod->klp_info section header information for livepatch modules
> >Does this mean we can drop the plt pointer from this struct altogether, and
> >simply offset into the section headers when applying the relocations?
>
> Hmm, if everyone is OK with dropping the plt pointer from struct
> mod_plt_sec, then I think we can simplify this patch even further.
>
> With the plt shndx saved, we can additionally pass a pointer to
> sechdrs to module_emit_plt_entry(), and with that just offset into the
> section headers as you suggest. Since livepatch *already* passes in
> the correct copy of the section headers (mod->klp_info->sechdrs) to
> apply_relocate_add(), we wouldn't even need to modify the arm64
> module_finalize() to change mod->arch.core.plt to point into
> mod->klp_info->sechdrs anymore and we can drop all the changes to the
> module loader too.
>
> Something like the following maybe?
>
> diff --git a/arch/arm64/include/asm/module.h b/arch/arm64/include/asm/module.h
> index fef773c94e9d..ac10fa066487 100644
> --- a/arch/arm64/include/asm/module.h
> +++ b/arch/arm64/include/asm/module.h
> @@ -22,7 +22,7 @@
>
> #ifdef CONFIG_ARM64_MODULE_PLTS
> struct mod_plt_sec {
> - struct elf64_shdr *plt;
> + int plt_shndx;
> int plt_num_entries;
> int plt_max_entries;
> };
> @@ -37,10 +37,12 @@ struct mod_arch_specific {
> };
> #endif
>
> -u64 module_emit_plt_entry(struct module *mod, void *loc, const Elf64_Rela
> *rela,
> +u64 module_emit_plt_entry(struct module *mod, Elf64_Shdr *sechdrs,
> + void *loc, const Elf64_Rela *rela,
> Elf64_Sym *sym);
>
> -u64 module_emit_veneer_for_adrp(struct module *mod, void *loc, u64 val);
> +u64 module_emit_veneer_for_adrp(struct module *mod, Elf64_Shdr *sechdrs,
> + void *loc, u64 val);
>
> #ifdef CONFIG_RANDOMIZE_BASE
> extern u64 module_alloc_base;
> diff --git a/arch/arm64/kernel/module-plts.c b/arch/arm64/kernel/module-plts.c
> index f0690c2ca3e0..3cd744a1cbc2 100644
> --- a/arch/arm64/kernel/module-plts.c
> +++ b/arch/arm64/kernel/module-plts.c
> @@ -16,13 +16,15 @@ static bool in_init(const struct module *mod, void *loc)
> return (u64)loc - (u64)mod->init_layout.base < mod->init_layout.size;
> }
>
> -u64 module_emit_plt_entry(struct module *mod, void *loc, const Elf64_Rela
> *rela,
> +u64 module_emit_plt_entry(struct module *mod, Elf64_Shdr *sechdrs,
> + void *loc, const Elf64_Rela *rela,
> Elf64_Sym *sym)
> {
> - struct mod_plt_sec *pltsec = !in_init(mod, loc) ? >arch.core :
> - >arch.init;
> - struct plt_entry *plt = (struct plt_entry *)pltsec->plt->sh_addr;
> - int i = pltsec->plt_num_entries;
> + struct mod_plt_sec *plt_info = !in_init(mod, loc) ? >arch.core :
> + >arch.init;
> + Elf64_Shdr *pltsec = sechdrs + plt_info->plt_shndx;
> + struct plt_entry *plt = (struct plt_entry *)pltsec->sh_addr;
> + int i = plt_info->plt_num_entries;
> u64 val = sym->st_value + rela->r_addend;
>
> plt[i] = get_plt_entry(val);
> @@ -35,24 +37,26 @@ u64 module_emit_plt_entry(struct module *mod, void *loc,
> const Elf64_Rela *rela,
> if (i > 0 && plt_entries_equal(plt + i, plt + i - 1))
> return (u64)[i - 1];
>
> - pltsec->plt_num_entries++;
> - if (WARN_ON(pltsec->plt_num_entries > pltsec->plt_max_entries))
> + plt_info->plt_num_entries++;
> + if (WARN_ON(plt_info->plt_num_entries > plt_info->plt_max_entries))
> return 0;
>
> return (u64)[i];
> }
>
> #ifdef CONFIG_ARM64_ERRATUM_843419
> -u64 module_emit_veneer_for_adrp(struct module *mod, void *loc, u64 val)
> +u64 module_emit_veneer_for_adrp(struct module *mod, Elf64_Shdr *sechdrs,
> + void *loc, u64 val)
> {
> - struct mod_plt_sec *pltsec = !in_init(mod, loc) ? >arch.core :
> - >arch.init;
> - struct plt_entry *plt = (struct plt_entry *)pltsec->plt->sh_addr;
> - int i = pltsec->plt_num_entries++;
> + struct mod_plt_sec *plt_info = !in_init(mod, loc) ? >arch.core :
> + >arch.init;
> + Elf64_Shdr *pltsec = sechdrs + plt_info->plt_shndx;
> + struct plt_entry *plt = (struct plt_entry *)pltsec->sh_addr;
> + int i = plt_info->plt_num_entries++;
> u32 mov0, mov1, mov2, br;
> int rd;
>
> - if (WARN_ON(pltsec->plt_num_entries > pltsec->plt_max_entries))
> + if (WARN_ON(plt_info->plt_num_entries > plt_info->plt_max_entries))
> return 0;
>
> /* get the destination register of the ADRP instruction */
> @@ -202,7 +206,7 @@ int module_frob_arch_sections(Elf_Ehdr *ehdr, Elf_Shdr
> *sechdrs,
> unsigned long core_plts = 0;
> unsigned long init_plts = 0;
> Elf64_Sym *syms =
Re: [PATCH v2] arm64/module: use mod->klp_info section header information for livepatch modules
> >Does this mean we can drop the plt pointer from this struct altogether, and
> >simply offset into the section headers when applying the relocations?
>
> Hmm, if everyone is OK with dropping the plt pointer from struct
> mod_plt_sec, then I think we can simplify this patch even further.
>
> With the plt shndx saved, we can additionally pass a pointer to
> sechdrs to module_emit_plt_entry(), and with that just offset into the
> section headers as you suggest. Since livepatch *already* passes in
> the correct copy of the section headers (mod->klp_info->sechdrs) to
> apply_relocate_add(), we wouldn't even need to modify the arm64
> module_finalize() to change mod->arch.core.plt to point into
> mod->klp_info->sechdrs anymore and we can drop all the changes to the
> module loader too.
>
> Something like the following maybe?
>
> diff --git a/arch/arm64/include/asm/module.h b/arch/arm64/include/asm/module.h
> index fef773c94e9d..ac10fa066487 100644
> --- a/arch/arm64/include/asm/module.h
> +++ b/arch/arm64/include/asm/module.h
> @@ -22,7 +22,7 @@
>
> #ifdef CONFIG_ARM64_MODULE_PLTS
> struct mod_plt_sec {
> - struct elf64_shdr *plt;
> + int plt_shndx;
> int plt_num_entries;
> int plt_max_entries;
> };
> @@ -37,10 +37,12 @@ struct mod_arch_specific {
> };
> #endif
>
> -u64 module_emit_plt_entry(struct module *mod, void *loc, const Elf64_Rela
> *rela,
> +u64 module_emit_plt_entry(struct module *mod, Elf64_Shdr *sechdrs,
> + void *loc, const Elf64_Rela *rela,
> Elf64_Sym *sym);
>
> -u64 module_emit_veneer_for_adrp(struct module *mod, void *loc, u64 val);
> +u64 module_emit_veneer_for_adrp(struct module *mod, Elf64_Shdr *sechdrs,
> + void *loc, u64 val);
>
> #ifdef CONFIG_RANDOMIZE_BASE
> extern u64 module_alloc_base;
> diff --git a/arch/arm64/kernel/module-plts.c b/arch/arm64/kernel/module-plts.c
> index f0690c2ca3e0..3cd744a1cbc2 100644
> --- a/arch/arm64/kernel/module-plts.c
> +++ b/arch/arm64/kernel/module-plts.c
> @@ -16,13 +16,15 @@ static bool in_init(const struct module *mod, void *loc)
> return (u64)loc - (u64)mod->init_layout.base < mod->init_layout.size;
> }
>
> -u64 module_emit_plt_entry(struct module *mod, void *loc, const Elf64_Rela
> *rela,
> +u64 module_emit_plt_entry(struct module *mod, Elf64_Shdr *sechdrs,
> + void *loc, const Elf64_Rela *rela,
> Elf64_Sym *sym)
> {
> - struct mod_plt_sec *pltsec = !in_init(mod, loc) ? >arch.core :
> - >arch.init;
> - struct plt_entry *plt = (struct plt_entry *)pltsec->plt->sh_addr;
> - int i = pltsec->plt_num_entries;
> + struct mod_plt_sec *plt_info = !in_init(mod, loc) ? >arch.core :
> + >arch.init;
> + Elf64_Shdr *pltsec = sechdrs + plt_info->plt_shndx;
> + struct plt_entry *plt = (struct plt_entry *)pltsec->sh_addr;
> + int i = plt_info->plt_num_entries;
> u64 val = sym->st_value + rela->r_addend;
>
> plt[i] = get_plt_entry(val);
> @@ -35,24 +37,26 @@ u64 module_emit_plt_entry(struct module *mod, void *loc,
> const Elf64_Rela *rela,
> if (i > 0 && plt_entries_equal(plt + i, plt + i - 1))
> return (u64)[i - 1];
>
> - pltsec->plt_num_entries++;
> - if (WARN_ON(pltsec->plt_num_entries > pltsec->plt_max_entries))
> + plt_info->plt_num_entries++;
> + if (WARN_ON(plt_info->plt_num_entries > plt_info->plt_max_entries))
> return 0;
>
> return (u64)[i];
> }
>
> #ifdef CONFIG_ARM64_ERRATUM_843419
> -u64 module_emit_veneer_for_adrp(struct module *mod, void *loc, u64 val)
> +u64 module_emit_veneer_for_adrp(struct module *mod, Elf64_Shdr *sechdrs,
> + void *loc, u64 val)
> {
> - struct mod_plt_sec *pltsec = !in_init(mod, loc) ? >arch.core :
> - >arch.init;
> - struct plt_entry *plt = (struct plt_entry *)pltsec->plt->sh_addr;
> - int i = pltsec->plt_num_entries++;
> + struct mod_plt_sec *plt_info = !in_init(mod, loc) ? >arch.core :
> + >arch.init;
> + Elf64_Shdr *pltsec = sechdrs + plt_info->plt_shndx;
> + struct plt_entry *plt = (struct plt_entry *)pltsec->sh_addr;
> + int i = plt_info->plt_num_entries++;
> u32 mov0, mov1, mov2, br;
> int rd;
>
> - if (WARN_ON(pltsec->plt_num_entries > pltsec->plt_max_entries))
> + if (WARN_ON(plt_info->plt_num_entries > plt_info->plt_max_entries))
> return 0;
>
> /* get the destination register of the ADRP instruction */
> @@ -202,7 +206,7 @@ int module_frob_arch_sections(Elf_Ehdr *ehdr, Elf_Shdr
> *sechdrs,
> unsigned long core_plts = 0;
> unsigned long init_plts = 0;
> Elf64_Sym *syms =
Re: [PATCH v2] arm64/module: use mod->klp_info section header information for livepatch modules
+++ Will Deacon [29/10/18 15:28 +]:
Hi Jessica,
On Fri, Oct 26, 2018 at 07:25:01PM +0200, Jessica Yu wrote:
The arm64 module loader keeps a pointer into info->sechdrs to keep track
of section header information for .plt section(s). A pointer to the
relevent section header (struct elf64_shdr) in info->sechdrs is stored
in mod->arch.{init,core}.plt. This pointer may be accessed while
applying relocations in apply_relocate_add() for example. And unlike
normal modules, livepatch modules can call apply_relocate_add() after
module load. But the info struct (and therefore info->sechdrs) gets
freed at the end of load_module() and so mod->arch.{init,core}.plt
becomes an invalid pointer after the module is done loading.
Luckily, livepatch modules already keep a copy of Elf section header
information in mod->klp_info. So make sure livepatch modules on arm64
have access to the section headers in klp_info and set
mod->arch.{init,core}.plt to the appropriate section header in
mod->klp_info so that they can call apply_relocate_add() even after
module load.
Signed-off-by: Jessica Yu
---
v2:
- fix missing free_module_elf() in error path
- move copy_module_elf() and module_finalize() out of post_relocation()
to make error handling more clear
- add braces to if-else block in arm64 module_frob_arch_sections()
arch/arm64/include/asm/module.h | 1 +
arch/arm64/kernel/module-plts.c | 17 -
arch/arm64/kernel/module.c | 10 ++
kernel/module.c | 29 +++--
4 files changed, 38 insertions(+), 19 deletions(-)
diff --git a/arch/arm64/include/asm/module.h b/arch/arm64/include/asm/module.h
index fef773c94e9d..ac9b97f9ae5e 100644
--- a/arch/arm64/include/asm/module.h
+++ b/arch/arm64/include/asm/module.h
@@ -25,6 +25,7 @@ struct mod_plt_sec {
struct elf64_shdr *plt;
int plt_num_entries;
int plt_max_entries;
+ int plt_shndx;
};
Does this mean we can drop the plt pointer from this struct altogether, and
simply offset into the section headers when applying the relocations?
Hmm, if everyone is OK with dropping the plt pointer from struct
mod_plt_sec, then I think we can simplify this patch even further.
With the plt shndx saved, we can additionally pass a pointer to
sechdrs to module_emit_plt_entry(), and with that just offset into the
section headers as you suggest. Since livepatch *already* passes in
the correct copy of the section headers (mod->klp_info->sechdrs) to
apply_relocate_add(), we wouldn't even need to modify the arm64
module_finalize() to change mod->arch.core.plt to point into
mod->klp_info->sechdrs anymore and we can drop all the changes to the
module loader too.
Something like the following maybe?
diff --git a/arch/arm64/include/asm/module.h b/arch/arm64/include/asm/module.h
index fef773c94e9d..ac10fa066487 100644
--- a/arch/arm64/include/asm/module.h
+++ b/arch/arm64/include/asm/module.h
@@ -22,7 +22,7 @@
#ifdef CONFIG_ARM64_MODULE_PLTS
struct mod_plt_sec {
- struct elf64_shdr *plt;
+ int plt_shndx;
int plt_num_entries;
int plt_max_entries;
};
@@ -37,10 +37,12 @@ struct mod_arch_specific {
};
#endif
-u64 module_emit_plt_entry(struct module *mod, void *loc, const Elf64_Rela
*rela,
+u64 module_emit_plt_entry(struct module *mod, Elf64_Shdr *sechdrs,
+ void *loc, const Elf64_Rela *rela,
Elf64_Sym *sym);
-u64 module_emit_veneer_for_adrp(struct module *mod, void *loc, u64 val);
+u64 module_emit_veneer_for_adrp(struct module *mod, Elf64_Shdr *sechdrs,
+ void *loc, u64 val);
#ifdef CONFIG_RANDOMIZE_BASE
extern u64 module_alloc_base;
diff --git a/arch/arm64/kernel/module-plts.c b/arch/arm64/kernel/module-plts.c
index f0690c2ca3e0..3cd744a1cbc2 100644
--- a/arch/arm64/kernel/module-plts.c
+++ b/arch/arm64/kernel/module-plts.c
@@ -16,13 +16,15 @@ static bool in_init(const struct module *mod, void *loc)
return (u64)loc - (u64)mod->init_layout.base < mod->init_layout.size;
}
-u64 module_emit_plt_entry(struct module *mod, void *loc, const Elf64_Rela
*rela,
+u64 module_emit_plt_entry(struct module *mod, Elf64_Shdr *sechdrs,
+ void *loc, const Elf64_Rela *rela,
Elf64_Sym *sym)
{
- struct mod_plt_sec *pltsec = !in_init(mod, loc) ? >arch.core :
- >arch.init;
- struct plt_entry *plt = (struct plt_entry *)pltsec->plt->sh_addr;
- int i = pltsec->plt_num_entries;
+ struct mod_plt_sec *plt_info = !in_init(mod, loc) ? >arch.core :
+ >arch.init;
+ Elf64_Shdr *pltsec = sechdrs + plt_info->plt_shndx;
+ struct plt_entry *plt = (struct plt_entry *)pltsec->sh_addr;
+ int i =
Re: [PATCH v2] arm64/module: use mod->klp_info section header information for livepatch modules
+++ Will Deacon [29/10/18 15:28 +]:
Hi Jessica,
On Fri, Oct 26, 2018 at 07:25:01PM +0200, Jessica Yu wrote:
The arm64 module loader keeps a pointer into info->sechdrs to keep track
of section header information for .plt section(s). A pointer to the
relevent section header (struct elf64_shdr) in info->sechdrs is stored
in mod->arch.{init,core}.plt. This pointer may be accessed while
applying relocations in apply_relocate_add() for example. And unlike
normal modules, livepatch modules can call apply_relocate_add() after
module load. But the info struct (and therefore info->sechdrs) gets
freed at the end of load_module() and so mod->arch.{init,core}.plt
becomes an invalid pointer after the module is done loading.
Luckily, livepatch modules already keep a copy of Elf section header
information in mod->klp_info. So make sure livepatch modules on arm64
have access to the section headers in klp_info and set
mod->arch.{init,core}.plt to the appropriate section header in
mod->klp_info so that they can call apply_relocate_add() even after
module load.
Signed-off-by: Jessica Yu
---
v2:
- fix missing free_module_elf() in error path
- move copy_module_elf() and module_finalize() out of post_relocation()
to make error handling more clear
- add braces to if-else block in arm64 module_frob_arch_sections()
arch/arm64/include/asm/module.h | 1 +
arch/arm64/kernel/module-plts.c | 17 -
arch/arm64/kernel/module.c | 10 ++
kernel/module.c | 29 +++--
4 files changed, 38 insertions(+), 19 deletions(-)
diff --git a/arch/arm64/include/asm/module.h b/arch/arm64/include/asm/module.h
index fef773c94e9d..ac9b97f9ae5e 100644
--- a/arch/arm64/include/asm/module.h
+++ b/arch/arm64/include/asm/module.h
@@ -25,6 +25,7 @@ struct mod_plt_sec {
struct elf64_shdr *plt;
int plt_num_entries;
int plt_max_entries;
+ int plt_shndx;
};
Does this mean we can drop the plt pointer from this struct altogether, and
simply offset into the section headers when applying the relocations?
Hmm, if everyone is OK with dropping the plt pointer from struct
mod_plt_sec, then I think we can simplify this patch even further.
With the plt shndx saved, we can additionally pass a pointer to
sechdrs to module_emit_plt_entry(), and with that just offset into the
section headers as you suggest. Since livepatch *already* passes in
the correct copy of the section headers (mod->klp_info->sechdrs) to
apply_relocate_add(), we wouldn't even need to modify the arm64
module_finalize() to change mod->arch.core.plt to point into
mod->klp_info->sechdrs anymore and we can drop all the changes to the
module loader too.
Something like the following maybe?
diff --git a/arch/arm64/include/asm/module.h b/arch/arm64/include/asm/module.h
index fef773c94e9d..ac10fa066487 100644
--- a/arch/arm64/include/asm/module.h
+++ b/arch/arm64/include/asm/module.h
@@ -22,7 +22,7 @@
#ifdef CONFIG_ARM64_MODULE_PLTS
struct mod_plt_sec {
- struct elf64_shdr *plt;
+ int plt_shndx;
int plt_num_entries;
int plt_max_entries;
};
@@ -37,10 +37,12 @@ struct mod_arch_specific {
};
#endif
-u64 module_emit_plt_entry(struct module *mod, void *loc, const Elf64_Rela
*rela,
+u64 module_emit_plt_entry(struct module *mod, Elf64_Shdr *sechdrs,
+ void *loc, const Elf64_Rela *rela,
Elf64_Sym *sym);
-u64 module_emit_veneer_for_adrp(struct module *mod, void *loc, u64 val);
+u64 module_emit_veneer_for_adrp(struct module *mod, Elf64_Shdr *sechdrs,
+ void *loc, u64 val);
#ifdef CONFIG_RANDOMIZE_BASE
extern u64 module_alloc_base;
diff --git a/arch/arm64/kernel/module-plts.c b/arch/arm64/kernel/module-plts.c
index f0690c2ca3e0..3cd744a1cbc2 100644
--- a/arch/arm64/kernel/module-plts.c
+++ b/arch/arm64/kernel/module-plts.c
@@ -16,13 +16,15 @@ static bool in_init(const struct module *mod, void *loc)
return (u64)loc - (u64)mod->init_layout.base < mod->init_layout.size;
}
-u64 module_emit_plt_entry(struct module *mod, void *loc, const Elf64_Rela
*rela,
+u64 module_emit_plt_entry(struct module *mod, Elf64_Shdr *sechdrs,
+ void *loc, const Elf64_Rela *rela,
Elf64_Sym *sym)
{
- struct mod_plt_sec *pltsec = !in_init(mod, loc) ? >arch.core :
- >arch.init;
- struct plt_entry *plt = (struct plt_entry *)pltsec->plt->sh_addr;
- int i = pltsec->plt_num_entries;
+ struct mod_plt_sec *plt_info = !in_init(mod, loc) ? >arch.core :
+ >arch.init;
+ Elf64_Shdr *pltsec = sechdrs + plt_info->plt_shndx;
+ struct plt_entry *plt = (struct plt_entry *)pltsec->sh_addr;
+ int i =
Re: [PATCH v2] arm64/module: use mod->klp_info section header information for livepatch modules
Hi Jessica,
On Fri, Oct 26, 2018 at 07:25:01PM +0200, Jessica Yu wrote:
> The arm64 module loader keeps a pointer into info->sechdrs to keep track
> of section header information for .plt section(s). A pointer to the
> relevent section header (struct elf64_shdr) in info->sechdrs is stored
> in mod->arch.{init,core}.plt. This pointer may be accessed while
> applying relocations in apply_relocate_add() for example. And unlike
> normal modules, livepatch modules can call apply_relocate_add() after
> module load. But the info struct (and therefore info->sechdrs) gets
> freed at the end of load_module() and so mod->arch.{init,core}.plt
> becomes an invalid pointer after the module is done loading.
>
> Luckily, livepatch modules already keep a copy of Elf section header
> information in mod->klp_info. So make sure livepatch modules on arm64
> have access to the section headers in klp_info and set
> mod->arch.{init,core}.plt to the appropriate section header in
> mod->klp_info so that they can call apply_relocate_add() even after
> module load.
>
> Signed-off-by: Jessica Yu
> ---
>
> v2:
> - fix missing free_module_elf() in error path
> - move copy_module_elf() and module_finalize() out of post_relocation()
>to make error handling more clear
> - add braces to if-else block in arm64 module_frob_arch_sections()
>
> arch/arm64/include/asm/module.h | 1 +
> arch/arm64/kernel/module-plts.c | 17 -
> arch/arm64/kernel/module.c | 10 ++
> kernel/module.c | 29 +++--
> 4 files changed, 38 insertions(+), 19 deletions(-)
>
> diff --git a/arch/arm64/include/asm/module.h b/arch/arm64/include/asm/module.h
> index fef773c94e9d..ac9b97f9ae5e 100644
> --- a/arch/arm64/include/asm/module.h
> +++ b/arch/arm64/include/asm/module.h
> @@ -25,6 +25,7 @@ struct mod_plt_sec {
> struct elf64_shdr *plt;
> int plt_num_entries;
> int plt_max_entries;
> + int plt_shndx;
> };
Does this mean we can drop the plt pointer from this struct altogether, and
simply offset into the section headers when applying the relocations?
Cheers,
Will
Re: [PATCH v2] arm64/module: use mod->klp_info section header information for livepatch modules
Hi Jessica,
On Fri, Oct 26, 2018 at 07:25:01PM +0200, Jessica Yu wrote:
> The arm64 module loader keeps a pointer into info->sechdrs to keep track
> of section header information for .plt section(s). A pointer to the
> relevent section header (struct elf64_shdr) in info->sechdrs is stored
> in mod->arch.{init,core}.plt. This pointer may be accessed while
> applying relocations in apply_relocate_add() for example. And unlike
> normal modules, livepatch modules can call apply_relocate_add() after
> module load. But the info struct (and therefore info->sechdrs) gets
> freed at the end of load_module() and so mod->arch.{init,core}.plt
> becomes an invalid pointer after the module is done loading.
>
> Luckily, livepatch modules already keep a copy of Elf section header
> information in mod->klp_info. So make sure livepatch modules on arm64
> have access to the section headers in klp_info and set
> mod->arch.{init,core}.plt to the appropriate section header in
> mod->klp_info so that they can call apply_relocate_add() even after
> module load.
>
> Signed-off-by: Jessica Yu
> ---
>
> v2:
> - fix missing free_module_elf() in error path
> - move copy_module_elf() and module_finalize() out of post_relocation()
>to make error handling more clear
> - add braces to if-else block in arm64 module_frob_arch_sections()
>
> arch/arm64/include/asm/module.h | 1 +
> arch/arm64/kernel/module-plts.c | 17 -
> arch/arm64/kernel/module.c | 10 ++
> kernel/module.c | 29 +++--
> 4 files changed, 38 insertions(+), 19 deletions(-)
>
> diff --git a/arch/arm64/include/asm/module.h b/arch/arm64/include/asm/module.h
> index fef773c94e9d..ac9b97f9ae5e 100644
> --- a/arch/arm64/include/asm/module.h
> +++ b/arch/arm64/include/asm/module.h
> @@ -25,6 +25,7 @@ struct mod_plt_sec {
> struct elf64_shdr *plt;
> int plt_num_entries;
> int plt_max_entries;
> + int plt_shndx;
> };
Does this mean we can drop the plt pointer from this struct altogether, and
simply offset into the section headers when applying the relocations?
Cheers,
Will
Re: [PATCH v2] arm64/module: use mod->klp_info section header information for livepatch modules
+++ Miroslav Benes [29/10/18 14:24 +0100]: diff --git a/arch/arm64/kernel/module.c b/arch/arm64/kernel/module.c index dd23655fda3a..490e56070a7e 100644 --- a/arch/arm64/kernel/module.c +++ b/arch/arm64/kernel/module.c @@ -461,5 +461,15 @@ int module_finalize(const Elf_Ehdr *hdr, #endif } +#ifdef CONFIG_LIVEPATCH + /* +* For livepatching, switch to the saved section header info for .plt +* stored in mod->klp_info. This is needed so that livepatch is able to +* call apply_relocate_add() after patch module load. +*/ + if (is_livepatch_module(me)) + me->arch.core.plt = me->klp_info->sechdrs + me->arch.core.plt_shndx; +#endif I missed it before, but the hunk should be under "#ifdef CONFIG_ARM64_MODULE_PLTS" protection similarly to ftrace_trampoline just above. me->arch.core.plt may not exist otherwise. Gah! Yes you are right, will fix. Thanks, Jessica
Re: [PATCH v2] arm64/module: use mod->klp_info section header information for livepatch modules
+++ Miroslav Benes [29/10/18 14:24 +0100]: diff --git a/arch/arm64/kernel/module.c b/arch/arm64/kernel/module.c index dd23655fda3a..490e56070a7e 100644 --- a/arch/arm64/kernel/module.c +++ b/arch/arm64/kernel/module.c @@ -461,5 +461,15 @@ int module_finalize(const Elf_Ehdr *hdr, #endif } +#ifdef CONFIG_LIVEPATCH + /* +* For livepatching, switch to the saved section header info for .plt +* stored in mod->klp_info. This is needed so that livepatch is able to +* call apply_relocate_add() after patch module load. +*/ + if (is_livepatch_module(me)) + me->arch.core.plt = me->klp_info->sechdrs + me->arch.core.plt_shndx; +#endif I missed it before, but the hunk should be under "#ifdef CONFIG_ARM64_MODULE_PLTS" protection similarly to ftrace_trampoline just above. me->arch.core.plt may not exist otherwise. Gah! Yes you are right, will fix. Thanks, Jessica
Re: [PATCH v2] arm64/module: use mod->klp_info section header information for livepatch modules
> diff --git a/arch/arm64/kernel/module.c b/arch/arm64/kernel/module.c > index dd23655fda3a..490e56070a7e 100644 > --- a/arch/arm64/kernel/module.c > +++ b/arch/arm64/kernel/module.c > @@ -461,5 +461,15 @@ int module_finalize(const Elf_Ehdr *hdr, > #endif > } > > +#ifdef CONFIG_LIVEPATCH > + /* > + * For livepatching, switch to the saved section header info for .plt > + * stored in mod->klp_info. This is needed so that livepatch is able to > + * call apply_relocate_add() after patch module load. > + */ > + if (is_livepatch_module(me)) > + me->arch.core.plt = me->klp_info->sechdrs + > me->arch.core.plt_shndx; > +#endif I missed it before, but the hunk should be under "#ifdef CONFIG_ARM64_MODULE_PLTS" protection similarly to ftrace_trampoline just above. me->arch.core.plt may not exist otherwise. Miroslav
Re: [PATCH v2] arm64/module: use mod->klp_info section header information for livepatch modules
> diff --git a/arch/arm64/kernel/module.c b/arch/arm64/kernel/module.c > index dd23655fda3a..490e56070a7e 100644 > --- a/arch/arm64/kernel/module.c > +++ b/arch/arm64/kernel/module.c > @@ -461,5 +461,15 @@ int module_finalize(const Elf_Ehdr *hdr, > #endif > } > > +#ifdef CONFIG_LIVEPATCH > + /* > + * For livepatching, switch to the saved section header info for .plt > + * stored in mod->klp_info. This is needed so that livepatch is able to > + * call apply_relocate_add() after patch module load. > + */ > + if (is_livepatch_module(me)) > + me->arch.core.plt = me->klp_info->sechdrs + > me->arch.core.plt_shndx; > +#endif I missed it before, but the hunk should be under "#ifdef CONFIG_ARM64_MODULE_PLTS" protection similarly to ftrace_trampoline just above. me->arch.core.plt may not exist otherwise. Miroslav
[PATCH v2] arm64/module: use mod->klp_info section header information for livepatch modules
The arm64 module loader keeps a pointer into info->sechdrs to keep track
of section header information for .plt section(s). A pointer to the
relevent section header (struct elf64_shdr) in info->sechdrs is stored
in mod->arch.{init,core}.plt. This pointer may be accessed while
applying relocations in apply_relocate_add() for example. And unlike
normal modules, livepatch modules can call apply_relocate_add() after
module load. But the info struct (and therefore info->sechdrs) gets
freed at the end of load_module() and so mod->arch.{init,core}.plt
becomes an invalid pointer after the module is done loading.
Luckily, livepatch modules already keep a copy of Elf section header
information in mod->klp_info. So make sure livepatch modules on arm64
have access to the section headers in klp_info and set
mod->arch.{init,core}.plt to the appropriate section header in
mod->klp_info so that they can call apply_relocate_add() even after
module load.
Signed-off-by: Jessica Yu
---
v2:
- fix missing free_module_elf() in error path
- move copy_module_elf() and module_finalize() out of post_relocation()
to make error handling more clear
- add braces to if-else block in arm64 module_frob_arch_sections()
arch/arm64/include/asm/module.h | 1 +
arch/arm64/kernel/module-plts.c | 17 -
arch/arm64/kernel/module.c | 10 ++
kernel/module.c | 29 +++--
4 files changed, 38 insertions(+), 19 deletions(-)
diff --git a/arch/arm64/include/asm/module.h b/arch/arm64/include/asm/module.h
index fef773c94e9d..ac9b97f9ae5e 100644
--- a/arch/arm64/include/asm/module.h
+++ b/arch/arm64/include/asm/module.h
@@ -25,6 +25,7 @@ struct mod_plt_sec {
struct elf64_shdr *plt;
int plt_num_entries;
int plt_max_entries;
+ int plt_shndx;
};
struct mod_arch_specific {
diff --git a/arch/arm64/kernel/module-plts.c b/arch/arm64/kernel/module-plts.c
index f0690c2ca3e0..851311ffd427 100644
--- a/arch/arm64/kernel/module-plts.c
+++ b/arch/arm64/kernel/module-plts.c
@@ -210,16 +210,23 @@ int module_frob_arch_sections(Elf_Ehdr *ehdr, Elf_Shdr
*sechdrs,
* entries. Record the symtab address as well.
*/
for (i = 0; i < ehdr->e_shnum; i++) {
- if (!strcmp(secstrings + sechdrs[i].sh_name, ".plt"))
+ if (!strcmp(secstrings + sechdrs[i].sh_name, ".plt")) {
mod->arch.core.plt = sechdrs + i;
- else if (!strcmp(secstrings + sechdrs[i].sh_name, ".init.plt"))
+ /*
+* Keep the section index for the .plt section for
+* livepatching. Note that .init.plt is irrelevant to
+* livepatch, so only the shndx for .plt is saved.
+*/
+ mod->arch.core.plt_shndx = i;
+ } else if (!strcmp(secstrings + sechdrs[i].sh_name,
".init.plt")) {
mod->arch.init.plt = sechdrs + i;
- else if (IS_ENABLED(CONFIG_DYNAMIC_FTRACE) &&
+ } else if (IS_ENABLED(CONFIG_DYNAMIC_FTRACE) &&
!strcmp(secstrings + sechdrs[i].sh_name,
-".text.ftrace_trampoline"))
+".text.ftrace_trampoline")) {
tramp = sechdrs + i;
- else if (sechdrs[i].sh_type == SHT_SYMTAB)
+ } else if (sechdrs[i].sh_type == SHT_SYMTAB) {
syms = (Elf64_Sym *)sechdrs[i].sh_addr;
+ }
}
if (!mod->arch.core.plt || !mod->arch.init.plt) {
diff --git a/arch/arm64/kernel/module.c b/arch/arm64/kernel/module.c
index dd23655fda3a..490e56070a7e 100644
--- a/arch/arm64/kernel/module.c
+++ b/arch/arm64/kernel/module.c
@@ -461,5 +461,15 @@ int module_finalize(const Elf_Ehdr *hdr,
#endif
}
+#ifdef CONFIG_LIVEPATCH
+ /*
+* For livepatching, switch to the saved section header info for .plt
+* stored in mod->klp_info. This is needed so that livepatch is able to
+* call apply_relocate_add() after patch module load.
+*/
+ if (is_livepatch_module(me))
+ me->arch.core.plt = me->klp_info->sechdrs +
me->arch.core.plt_shndx;
+#endif
+
return 0;
}
diff --git a/kernel/module.c b/kernel/module.c
index f475f30eed8c..611f4fe64370 100644
--- a/kernel/module.c
+++ b/kernel/module.c
@@ -3365,7 +3365,7 @@ int __weak module_finalize(const Elf_Ehdr *hdr,
return 0;
}
-static int post_relocation(struct module *mod, const struct load_info *info)
+static void post_relocation(struct module *mod, const struct load_info *info)
{
/* Sort exception table now relocations are done. */
sort_extable(mod->extable, mod->extable + mod->num_exentries);
@@ -3376,9 +3376,6 @@ static int post_relocation(struct module *mod, const
struct load_info *info)
[PATCH v2] arm64/module: use mod->klp_info section header information for livepatch modules
The arm64 module loader keeps a pointer into info->sechdrs to keep track
of section header information for .plt section(s). A pointer to the
relevent section header (struct elf64_shdr) in info->sechdrs is stored
in mod->arch.{init,core}.plt. This pointer may be accessed while
applying relocations in apply_relocate_add() for example. And unlike
normal modules, livepatch modules can call apply_relocate_add() after
module load. But the info struct (and therefore info->sechdrs) gets
freed at the end of load_module() and so mod->arch.{init,core}.plt
becomes an invalid pointer after the module is done loading.
Luckily, livepatch modules already keep a copy of Elf section header
information in mod->klp_info. So make sure livepatch modules on arm64
have access to the section headers in klp_info and set
mod->arch.{init,core}.plt to the appropriate section header in
mod->klp_info so that they can call apply_relocate_add() even after
module load.
Signed-off-by: Jessica Yu
---
v2:
- fix missing free_module_elf() in error path
- move copy_module_elf() and module_finalize() out of post_relocation()
to make error handling more clear
- add braces to if-else block in arm64 module_frob_arch_sections()
arch/arm64/include/asm/module.h | 1 +
arch/arm64/kernel/module-plts.c | 17 -
arch/arm64/kernel/module.c | 10 ++
kernel/module.c | 29 +++--
4 files changed, 38 insertions(+), 19 deletions(-)
diff --git a/arch/arm64/include/asm/module.h b/arch/arm64/include/asm/module.h
index fef773c94e9d..ac9b97f9ae5e 100644
--- a/arch/arm64/include/asm/module.h
+++ b/arch/arm64/include/asm/module.h
@@ -25,6 +25,7 @@ struct mod_plt_sec {
struct elf64_shdr *plt;
int plt_num_entries;
int plt_max_entries;
+ int plt_shndx;
};
struct mod_arch_specific {
diff --git a/arch/arm64/kernel/module-plts.c b/arch/arm64/kernel/module-plts.c
index f0690c2ca3e0..851311ffd427 100644
--- a/arch/arm64/kernel/module-plts.c
+++ b/arch/arm64/kernel/module-plts.c
@@ -210,16 +210,23 @@ int module_frob_arch_sections(Elf_Ehdr *ehdr, Elf_Shdr
*sechdrs,
* entries. Record the symtab address as well.
*/
for (i = 0; i < ehdr->e_shnum; i++) {
- if (!strcmp(secstrings + sechdrs[i].sh_name, ".plt"))
+ if (!strcmp(secstrings + sechdrs[i].sh_name, ".plt")) {
mod->arch.core.plt = sechdrs + i;
- else if (!strcmp(secstrings + sechdrs[i].sh_name, ".init.plt"))
+ /*
+* Keep the section index for the .plt section for
+* livepatching. Note that .init.plt is irrelevant to
+* livepatch, so only the shndx for .plt is saved.
+*/
+ mod->arch.core.plt_shndx = i;
+ } else if (!strcmp(secstrings + sechdrs[i].sh_name,
".init.plt")) {
mod->arch.init.plt = sechdrs + i;
- else if (IS_ENABLED(CONFIG_DYNAMIC_FTRACE) &&
+ } else if (IS_ENABLED(CONFIG_DYNAMIC_FTRACE) &&
!strcmp(secstrings + sechdrs[i].sh_name,
-".text.ftrace_trampoline"))
+".text.ftrace_trampoline")) {
tramp = sechdrs + i;
- else if (sechdrs[i].sh_type == SHT_SYMTAB)
+ } else if (sechdrs[i].sh_type == SHT_SYMTAB) {
syms = (Elf64_Sym *)sechdrs[i].sh_addr;
+ }
}
if (!mod->arch.core.plt || !mod->arch.init.plt) {
diff --git a/arch/arm64/kernel/module.c b/arch/arm64/kernel/module.c
index dd23655fda3a..490e56070a7e 100644
--- a/arch/arm64/kernel/module.c
+++ b/arch/arm64/kernel/module.c
@@ -461,5 +461,15 @@ int module_finalize(const Elf_Ehdr *hdr,
#endif
}
+#ifdef CONFIG_LIVEPATCH
+ /*
+* For livepatching, switch to the saved section header info for .plt
+* stored in mod->klp_info. This is needed so that livepatch is able to
+* call apply_relocate_add() after patch module load.
+*/
+ if (is_livepatch_module(me))
+ me->arch.core.plt = me->klp_info->sechdrs +
me->arch.core.plt_shndx;
+#endif
+
return 0;
}
diff --git a/kernel/module.c b/kernel/module.c
index f475f30eed8c..611f4fe64370 100644
--- a/kernel/module.c
+++ b/kernel/module.c
@@ -3365,7 +3365,7 @@ int __weak module_finalize(const Elf_Ehdr *hdr,
return 0;
}
-static int post_relocation(struct module *mod, const struct load_info *info)
+static void post_relocation(struct module *mod, const struct load_info *info)
{
/* Sort exception table now relocations are done. */
sort_extable(mod->extable, mod->extable + mod->num_exentries);
@@ -3376,9 +3376,6 @@ static int post_relocation(struct module *mod, const
struct load_info *info)
