| Message ID | 20181001141652.5478C68BE1@newverein.lst.de (mailing list archive) |
|---|---|
| State | Superseded, archived |
| Headers | show |
| Series | arm64 live patching | expand |
On Mon, 1 Oct 2018, Torsten Duwe wrote: > Based on ftrace with regs, do the usual thing. Also allocate a > task flag for whatever consistency handling will be used. > Watch out for interactions with the graph tracer. Similar to what Mark wrote about 2/4, I'd appreciate a better commit log. Could you explain traditional "what/why/how", please? > Signed-off-by: Torsten Duwe <duwe@suse.de> > > --- a/arch/arm64/Kconfig > +++ b/arch/arm64/Kconfig > @@ -119,6 +119,7 @@ config ARM64 > select HAVE_GENERIC_DMA_COHERENT > select HAVE_HW_BREAKPOINT if PERF_EVENTS > select HAVE_IRQ_TIME_ACCOUNTING > + select HAVE_LIVEPATCH > select HAVE_MEMBLOCK > select HAVE_MEMBLOCK_NODE_MAP if NUMA > select HAVE_NMI > @@ -1349,4 +1350,6 @@ if CRYPTO > source "arch/arm64/crypto/Kconfig" > endif > > +source "kernel/livepatch/Kconfig" > + > source "lib/Kconfig" > --- a/arch/arm64/include/asm/thread_info.h > +++ b/arch/arm64/include/asm/thread_info.h > @@ -76,6 +76,7 @@ void arch_release_task_struct(struct tas > #define TIF_FOREIGN_FPSTATE 3 /* CPU's FP state is not current's */ > #define TIF_UPROBE 4 /* uprobe breakpoint or singlestep */ > #define TIF_FSCHECK 5 /* Check FS is USER_DS on return */ > +#define TIF_PATCH_PENDING 6 > #define TIF_NOHZ 7 > #define TIF_SYSCALL_TRACE 8 > #define TIF_SYSCALL_AUDIT 9 > @@ -94,6 +95,7 @@ void arch_release_task_struct(struct tas > #define _TIF_NEED_RESCHED (1 << TIF_NEED_RESCHED) > #define _TIF_NOTIFY_RESUME (1 << TIF_NOTIFY_RESUME) > #define _TIF_FOREIGN_FPSTATE (1 << TIF_FOREIGN_FPSTATE) > +#define _TIF_PATCH_PENDING (1 << TIF_PATCH_PENDING) > #define _TIF_NOHZ (1 << TIF_NOHZ) > #define _TIF_SYSCALL_TRACE (1 << TIF_SYSCALL_TRACE) > #define _TIF_SYSCALL_AUDIT (1 << TIF_SYSCALL_AUDIT) > @@ -106,7 +108,8 @@ void arch_release_task_struct(struct tas > > #define _TIF_WORK_MASK (_TIF_NEED_RESCHED | _TIF_SIGPENDING | \ > _TIF_NOTIFY_RESUME | _TIF_FOREIGN_FPSTATE | \ > - _TIF_UPROBE | _TIF_FSCHECK) > + _TIF_UPROBE | _TIF_FSCHECK | \ > + _TIF_PATCH_PENDING) Could you add a note to the changelog what this means? My ability to read arm64 entry.S is very limited, but I can see that _TIF_WORK_MASK is process in a syscall exit and irq return paths. That's good. It is also called (via "b ret_to_user") in a couple of different places (el0_* labels). I guess those are returns from exception handling. A comment about it in the changelog would be appreciated. > #define _TIF_SYSCALL_WORK (_TIF_SYSCALL_TRACE | _TIF_SYSCALL_AUDIT | \ > _TIF_SYSCALL_TRACEPOINT | _TIF_SECCOMP | \ > --- /dev/null > +++ b/arch/arm64/include/asm/livepatch.h > @@ -0,0 +1,36 @@ > +/* SPDX-License-Identifier: GPL-2.0 > + * > + * livepatch.h - arm64-specific Kernel Live Patching Core > + * > + * Copyright (C) 2016,2018 SUSE > + * > + * This program is free software; you can redistribute it and/or > + * modify it under the terms of the GNU General Public License > + * as published by the Free Software Foundation; either version 2 > + * of the License, or (at your option) any later version. > + * > + * This program is distributed in the hope that it will be useful, > + * but WITHOUT ANY WARRANTY; without even the implied warranty of > + * MERCHANTABILITY or FITNESS FOR A PARTICULAR PURPOSE. See the > + * GNU General Public License for more details. > + * > + * You should have received a copy of the GNU General Public License > + * along with this program; if not, see <http://www.gnu.org/licenses/>. > + */ > +#ifndef _ASM_ARM64_LIVEPATCH_H > +#define _ASM_ARM64_LIVEPATCH_H > + > +#include <linux/module.h> > +#include <linux/ftrace.h> I think that only #include <asm/ptrace.h> is in fact needed, because of "struct pt_regs". Ad relocations. I checked that everything in struct mod_arch_specific stays after the module is load. Both core and init get SHF_ALLOC set (mod->arch.core.plt->sh_flags in module_frob_arch_sections(). It is important because apply_relocate_add() may use those sections through module_emit_plt_entry() call. ftrace_trampoline section gets SHF_ALLOC as well. Btw, don't you want to do something similar for new ftrace_regs_trampoline in module_frob_arch_sections()? Perhaps even edit arch/arm64/kernel/module.lds? I'm completely clueless here, so it may be ok. And it applies to 2/4 patch. The last thing is count_plts() function called from module_frob_arch_sections(). It needed to be changed in 2016 as well. See Jessica's patch (20160713001113.GA30925@packer-debian-8-amd64.digitalocean.com). The logic in the function has changed since then. If I am not mistaken, count_plts() is fine as it is right now. It does not consider SHN_UNDEF anymore, it looks at the destination section (where a symbol should resolved to) only. Jessica, could you doublecheck, please? Torsten, please doublecheck all this as well and add a comment about all this to the changelog, so we don't have to analyze it from the beginning again in the future. Thanks, Miroslav
+++ Miroslav Benes [17/10/18 15:39 +0200]: >On Mon, 1 Oct 2018, Torsten Duwe wrote: > >> Based on ftrace with regs, do the usual thing. Also allocate a >> task flag for whatever consistency handling will be used. >> Watch out for interactions with the graph tracer. > >Similar to what Mark wrote about 2/4, I'd appreciate a better commit log. >Could you explain traditional "what/why/how", please? > >> Signed-off-by: Torsten Duwe <duwe@suse.de> >> >> --- a/arch/arm64/Kconfig >> +++ b/arch/arm64/Kconfig >> @@ -119,6 +119,7 @@ config ARM64 >> select HAVE_GENERIC_DMA_COHERENT >> select HAVE_HW_BREAKPOINT if PERF_EVENTS >> select HAVE_IRQ_TIME_ACCOUNTING >> + select HAVE_LIVEPATCH >> select HAVE_MEMBLOCK >> select HAVE_MEMBLOCK_NODE_MAP if NUMA >> select HAVE_NMI >> @@ -1349,4 +1350,6 @@ if CRYPTO >> source "arch/arm64/crypto/Kconfig" >> endif >> >> +source "kernel/livepatch/Kconfig" >> + >> source "lib/Kconfig" >> --- a/arch/arm64/include/asm/thread_info.h >> +++ b/arch/arm64/include/asm/thread_info.h >> @@ -76,6 +76,7 @@ void arch_release_task_struct(struct tas >> #define TIF_FOREIGN_FPSTATE 3 /* CPU's FP state is not current's */ >> #define TIF_UPROBE 4 /* uprobe breakpoint or singlestep */ >> #define TIF_FSCHECK 5 /* Check FS is USER_DS on return */ >> +#define TIF_PATCH_PENDING 6 >> #define TIF_NOHZ 7 >> #define TIF_SYSCALL_TRACE 8 >> #define TIF_SYSCALL_AUDIT 9 >> @@ -94,6 +95,7 @@ void arch_release_task_struct(struct tas >> #define _TIF_NEED_RESCHED (1 << TIF_NEED_RESCHED) >> #define _TIF_NOTIFY_RESUME (1 << TIF_NOTIFY_RESUME) >> #define _TIF_FOREIGN_FPSTATE (1 << TIF_FOREIGN_FPSTATE) >> +#define _TIF_PATCH_PENDING (1 << TIF_PATCH_PENDING) >> #define _TIF_NOHZ (1 << TIF_NOHZ) >> #define _TIF_SYSCALL_TRACE (1 << TIF_SYSCALL_TRACE) >> #define _TIF_SYSCALL_AUDIT (1 << TIF_SYSCALL_AUDIT) >> @@ -106,7 +108,8 @@ void arch_release_task_struct(struct tas >> >> #define _TIF_WORK_MASK (_TIF_NEED_RESCHED | _TIF_SIGPENDING | \ >> _TIF_NOTIFY_RESUME | _TIF_FOREIGN_FPSTATE | \ >> - _TIF_UPROBE | _TIF_FSCHECK) >> + _TIF_UPROBE | _TIF_FSCHECK | \ >> + _TIF_PATCH_PENDING) > >Could you add a note to the changelog what this means? My ability to read >arm64 entry.S is very limited, but I can see that _TIF_WORK_MASK is >process in a syscall exit and irq return paths. That's good. It is also >called (via "b ret_to_user") in a couple of different places (el0_* >labels). I guess those are returns from exception handling. A comment >about it in the changelog would be appreciated. > >> #define _TIF_SYSCALL_WORK (_TIF_SYSCALL_TRACE | _TIF_SYSCALL_AUDIT | \ >> _TIF_SYSCALL_TRACEPOINT | _TIF_SECCOMP | \ >> --- /dev/null >> +++ b/arch/arm64/include/asm/livepatch.h >> @@ -0,0 +1,36 @@ >> +/* SPDX-License-Identifier: GPL-2.0 >> + * >> + * livepatch.h - arm64-specific Kernel Live Patching Core >> + * >> + * Copyright (C) 2016,2018 SUSE >> + * >> + * This program is free software; you can redistribute it and/or >> + * modify it under the terms of the GNU General Public License >> + * as published by the Free Software Foundation; either version 2 >> + * of the License, or (at your option) any later version. >> + * >> + * This program is distributed in the hope that it will be useful, >> + * but WITHOUT ANY WARRANTY; without even the implied warranty of >> + * MERCHANTABILITY or FITNESS FOR A PARTICULAR PURPOSE. See the >> + * GNU General Public License for more details. >> + * >> + * You should have received a copy of the GNU General Public License >> + * along with this program; if not, see <http://www.gnu.org/licenses/>. >> + */ >> +#ifndef _ASM_ARM64_LIVEPATCH_H >> +#define _ASM_ARM64_LIVEPATCH_H >> + >> +#include <linux/module.h> >> +#include <linux/ftrace.h> > >I think that only > >#include <asm/ptrace.h> > >is in fact needed, because of "struct pt_regs". > > >Ad relocations. I checked that everything in struct mod_arch_specific >stays after the module is load. Both core and init get SHF_ALLOC set >(mod->arch.core.plt->sh_flags in module_frob_arch_sections(). It is >important because apply_relocate_add() may use those sections >through module_emit_plt_entry() call. Yes, it looks like the needed .plt sections will remain in module memory. However, I think I found a slight issue... :/ In module_frob_arch_sections(), mod->arch.core.plt is set to an offset within info->sechdrs: if (!strcmp(secstrings + sechdrs[i].sh_name, ".plt")) mod->arch.core.plt = sechdrs + i; sechdrs is from info->sechdrs, which is freed at the end of load_module() via free_copy(). So although the relevant plt section(s) are in module memory, mod->arch.core.plt will point to invalid memory after info is freed. In other words, the section (.plt) *is* in memory but the section header (struct elf64_shdr) containing section metadata like sh_addr, sh_size etc., is not. But we have mod->klp_info->sechdrs (which is a copy of the section headers) for this very reason. It is initialized only at the very end of load_module(). I don't think copy_module_elf() is dependent on anything, so it can just be moved earlier. Note that we cannot set mod->arch.core.plt to mod->klp_info->sechdrs + i during module_frob_arch_sections() because it is still too early, none of the module sections have been copied and none of their sh_addr's have been set to their final locations as this is all happening before move_module() is called. So we can use a module_finalize() function for arm64 to switch the mod->arch.core.plt to use the klp_info section headers. Maybe this will be more clear in the example fix below (completely untested and unreviewed!): diff --git a/arch/arm64/include/asm/module.h b/arch/arm64/include/asm/module.h index 97d0ef12e2ff..150afc29171b 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..c23cef8f0165 100644 --- a/arch/arm64/kernel/module-plts.c +++ b/arch/arm64/kernel/module-plts.c @@ -196,6 +196,21 @@ static unsigned int count_plts(Elf64_Sym *syms, Elf64_Rela *rela, int num, return ret; } +int module_finalize(const Elf_Ehdr *hdr, const Elf_Shdr *sechdrs, + struct module *mod) +{ + /* + * Livepatch modules need to keep access to section headers to apply + * relocations. Note mod->klp_info should have already been initialized + * and all section sh_addr's should have their final addresses by the + * time module_finalize() is called. + */ + if (is_livepatch_module(mod)) + mod->arch.core.plt = mod->klp_info->sechdrs + mod->arch.core.plt_shndx; + + return 0; +} + int module_frob_arch_sections(Elf_Ehdr *ehdr, Elf_Shdr *sechdrs, char *secstrings, struct module *mod) { @@ -210,11 +225,13 @@ 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")) + 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) && + mod->arch.init.plt_shndx = i; + } else if (IS_ENABLED(CONFIG_DYNAMIC_FTRACE) && !strcmp(secstrings + sechdrs[i].sh_name, ".text.ftrace_trampoline")) tramp = sechdrs + i; diff --git a/kernel/module.c b/kernel/module.c index 6746c85511fe..2fc4d74288dd 100644 --- a/kernel/module.c +++ b/kernel/module.c @@ -3363,6 +3363,12 @@ static int post_relocation(struct module *mod, const struct load_info *info) /* Setup kallsyms-specific fields. */ add_kallsyms(mod, info); + if (is_livepatch_module(mod)) { + err = copy_module_elf(mod, info); + if (err < 0) + return err; + } + /* Arch-specific module finalizing. */ return module_finalize(info->hdr, info->sechdrs, mod); } @@ -3775,12 +3781,6 @@ static int load_module(struct load_info *info, const char __user *uargs, if (err < 0) goto coming_cleanup; - if (is_livepatch_module(mod)) { - err = copy_module_elf(mod, info); - if (err < 0) - goto sysfs_cleanup; - } - /* Get rid of temporary copy. */ free_copy(info); Thoughts? Does the fix make sense? >The last thing is count_plts() function called from >module_frob_arch_sections(). It needed to be changed in 2016 as well. See >Jessica's patch >(20160713001113.GA30925@packer-debian-8-amd64.digitalocean.com). The logic >in the function has changed since then. If I am not mistaken, >count_plts() is fine as it is right now. It does not consider SHN_UNDEF >anymore, it looks at the destination section (where a symbol should >resolved to) only. > >Jessica, could you doublecheck, please? Yes, I think count_plts() is fine now in this case (because it is always true that sym->st_shndx != dstidx for SHN_LIVEPATCH symbols) and my old patch from 2016 is no longer needed. Thanks, Jessica
On Thu, 18 Oct 2018, Jessica Yu wrote: > +++ Miroslav Benes [17/10/18 15:39 +0200]: > >On Mon, 1 Oct 2018, Torsten Duwe wrote: > > > >Ad relocations. I checked that everything in struct mod_arch_specific > >stays after the module is load. Both core and init get SHF_ALLOC set > >(mod->arch.core.plt->sh_flags in module_frob_arch_sections(). It is > >important because apply_relocate_add() may use those sections > >through module_emit_plt_entry() call. > > Yes, it looks like the needed .plt sections will remain in module > memory. However, I think I found a slight issue... :/ > > In module_frob_arch_sections(), mod->arch.core.plt is set to an offset > within info->sechdrs: > > if (!strcmp(secstrings + sechdrs[i].sh_name, ".plt")) > mod->arch.core.plt = sechdrs + i; > > sechdrs is from info->sechdrs, which is freed at the end of > load_module() via free_copy(). So although the relevant plt section(s) > are in module memory, mod->arch.core.plt will point to invalid memory > after info is freed. In other words, the section (.plt) *is* in memory > but the section header (struct elf64_shdr) containing section metadata > like sh_addr, sh_size etc., is not. > > But we have mod->klp_info->sechdrs (which is a copy of the section > headers) for this very reason. It is initialized only at the very end > of load_module(). I don't think copy_module_elf() is dependent on > anything, so it can just be moved earlier. > > Note that we cannot set mod->arch.core.plt to mod->klp_info->sechdrs + i > during module_frob_arch_sections() because it is still too early, none > of the module sections have been copied and none of their sh_addr's > have been set to their final locations as this is all happening before > move_module() is called. So we can use a module_finalize() function > for arm64 to switch the mod->arch.core.plt to use the klp_info section > headers. Thanks for the analysis. You seem to be right. > Maybe this will be more clear in the example fix below (completely > untested and unreviewed!): > > diff --git a/arch/arm64/include/asm/module.h b/arch/arm64/include/asm/module.h > index 97d0ef12e2ff..150afc29171b 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..c23cef8f0165 100644 > --- a/arch/arm64/kernel/module-plts.c > +++ b/arch/arm64/kernel/module-plts.c > @@ -196,6 +196,21 @@ static unsigned int count_plts(Elf64_Sym *syms, > Elf64_Rela *rela, int num, > return ret; > } > > +int module_finalize(const Elf_Ehdr *hdr, const Elf_Shdr *sechdrs, > + struct module *mod) > +{ > + /* > + * Livepatch modules need to keep access to section headers to apply > + * relocations. Note mod->klp_info should have already been > initialized > + * and all section sh_addr's should have their final addresses by the > + * time module_finalize() is called. > + */ > + if (is_livepatch_module(mod)) > + mod->arch.core.plt = mod->klp_info->sechdrs + > mod->arch.core.plt_shndx; > + > + return 0; > +} There is already module_finalize() in arch/arm64/kernel/module.c, so this should probably go there. If I am not mistaken, we do not care for arch.init.plt in livepatch. Is that correct? > int module_frob_arch_sections(Elf_Ehdr *ehdr, Elf_Shdr *sechdrs, > char *secstrings, struct module *mod) > { > @@ -210,11 +225,13 @@ 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")) > + 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) && > + mod->arch.init.plt_shndx = i; It is initialized here, but that's not necessarily a bad thing. > + } else if (IS_ENABLED(CONFIG_DYNAMIC_FTRACE) && > !strcmp(secstrings + sechdrs[i].sh_name, > ".text.ftrace_trampoline")) > tramp = sechdrs + i; > diff --git a/kernel/module.c b/kernel/module.c > index 6746c85511fe..2fc4d74288dd 100644 > --- a/kernel/module.c > +++ b/kernel/module.c > @@ -3363,6 +3363,12 @@ static int post_relocation(struct module *mod, const > struct load_info *info) > /* Setup kallsyms-specific fields. */ > add_kallsyms(mod, info); > > + if (is_livepatch_module(mod)) { > + err = copy_module_elf(mod, info); > + if (err < 0) > + return err; > + } > + > /* Arch-specific module finalizing. */ > return module_finalize(info->hdr, info->sechdrs, mod); > } > @@ -3775,12 +3781,6 @@ static int load_module(struct load_info *info, const > char __user *uargs, > if (err < 0) > goto coming_cleanup; > > - if (is_livepatch_module(mod)) { > - err = copy_module_elf(mod, info); > - if (err < 0) > - goto sysfs_cleanup; > - } > - > /* Get rid of temporary copy. */ > free_copy(info); Hm, this is hopefully all right. We'd need to change the error handling a bit. free_module_elf() is now called in free_module() and it should be moved somewhere to load_module(). Probably under free_arch_cleanup label. Although it would be much better to rework post_relocation() and handle it right there. Something like - return module_finalize(info->hdr, info->sechdrs, mod); + err = module_finalize(info->hdr, info->sechdrs, mod); + if (err < 0) + free_module_elf(); + + return err; > Thoughts? Does the fix make sense? I think so. > >The last thing is count_plts() function called from > >module_frob_arch_sections(). It needed to be changed in 2016 as well. See > >Jessica's patch > >(20160713001113.GA30925@packer-debian-8-amd64.digitalocean.com). The logic > >in the function has changed since then. If I am not mistaken, > >count_plts() is fine as it is right now. It does not consider SHN_UNDEF > >anymore, it looks at the destination section (where a symbol should > >resolved to) only. > > > >Jessica, could you doublecheck, please? > > Yes, I think count_plts() is fine now in this case (because it is > always true that sym->st_shndx != dstidx for SHN_LIVEPATCH symbols) > and my old patch from 2016 is no longer needed. Thanks a lot, Jessica. Torsten, could you include the outcome to your patch set once we settle on it? Thanks. Miroslav
+++ Miroslav Benes [19/10/18 13:59 +0200]: >On Thu, 18 Oct 2018, Jessica Yu wrote: > >> +++ Miroslav Benes [17/10/18 15:39 +0200]: >> >On Mon, 1 Oct 2018, Torsten Duwe wrote: >> > >> >Ad relocations. I checked that everything in struct mod_arch_specific >> >stays after the module is load. Both core and init get SHF_ALLOC set >> >(mod->arch.core.plt->sh_flags in module_frob_arch_sections(). It is >> >important because apply_relocate_add() may use those sections >> >through module_emit_plt_entry() call. >> >> Yes, it looks like the needed .plt sections will remain in module >> memory. However, I think I found a slight issue... :/ >> >> In module_frob_arch_sections(), mod->arch.core.plt is set to an offset >> within info->sechdrs: >> >> if (!strcmp(secstrings + sechdrs[i].sh_name, ".plt")) >> mod->arch.core.plt = sechdrs + i; >> >> sechdrs is from info->sechdrs, which is freed at the end of >> load_module() via free_copy(). So although the relevant plt section(s) >> are in module memory, mod->arch.core.plt will point to invalid memory >> after info is freed. In other words, the section (.plt) *is* in memory >> but the section header (struct elf64_shdr) containing section metadata >> like sh_addr, sh_size etc., is not. >> >> But we have mod->klp_info->sechdrs (which is a copy of the section >> headers) for this very reason. It is initialized only at the very end >> of load_module(). I don't think copy_module_elf() is dependent on >> anything, so it can just be moved earlier. >> >> Note that we cannot set mod->arch.core.plt to mod->klp_info->sechdrs + i >> during module_frob_arch_sections() because it is still too early, none >> of the module sections have been copied and none of their sh_addr's >> have been set to their final locations as this is all happening before >> move_module() is called. So we can use a module_finalize() function >> for arm64 to switch the mod->arch.core.plt to use the klp_info section >> headers. > >Thanks for the analysis. You seem to be right. > >> Maybe this will be more clear in the example fix below (completely >> untested and unreviewed!): >> >> diff --git a/arch/arm64/include/asm/module.h b/arch/arm64/include/asm/module.h >> index 97d0ef12e2ff..150afc29171b 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..c23cef8f0165 100644 >> --- a/arch/arm64/kernel/module-plts.c >> +++ b/arch/arm64/kernel/module-plts.c >> @@ -196,6 +196,21 @@ static unsigned int count_plts(Elf64_Sym *syms, >> Elf64_Rela *rela, int num, >> return ret; >> } >> >> +int module_finalize(const Elf_Ehdr *hdr, const Elf_Shdr *sechdrs, >> + struct module *mod) >> +{ >> + /* >> + * Livepatch modules need to keep access to section headers to apply >> + * relocations. Note mod->klp_info should have already been >> initialized >> + * and all section sh_addr's should have their final addresses by the >> + * time module_finalize() is called. >> + */ >> + if (is_livepatch_module(mod)) >> + mod->arch.core.plt = mod->klp_info->sechdrs + >> mod->arch.core.plt_shndx; >> + >> + return 0; >> +} > >There is already module_finalize() in arch/arm64/kernel/module.c, so this >should probably go there. Ah yeah, I missed that :) Yep, that would be where it'd go. >If I am not mistaken, we do not care for arch.init.plt in livepatch. Is >that correct? I do not believe patching of __init functions is supported (right?) So we do not need to keep arch.init.plt alive post-module-load. >> int module_frob_arch_sections(Elf_Ehdr *ehdr, Elf_Shdr *sechdrs, >> char *secstrings, struct module *mod) >> { >> @@ -210,11 +225,13 @@ 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")) >> + 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) && >> + mod->arch.init.plt_shndx = i; > >It is initialized here, but that's not necessarily a bad thing. I think I added this line for consistency, but I actually don't think it is needed. We only would need to keep the section index for arch.core.plt then. >> + } else if (IS_ENABLED(CONFIG_DYNAMIC_FTRACE) && >> !strcmp(secstrings + sechdrs[i].sh_name, >> ".text.ftrace_trampoline")) >> tramp = sechdrs + i; >> diff --git a/kernel/module.c b/kernel/module.c >> index 6746c85511fe..2fc4d74288dd 100644 >> --- a/kernel/module.c >> +++ b/kernel/module.c >> @@ -3363,6 +3363,12 @@ static int post_relocation(struct module *mod, const >> struct load_info *info) >> /* Setup kallsyms-specific fields. */ >> add_kallsyms(mod, info); >> >> + if (is_livepatch_module(mod)) { >> + err = copy_module_elf(mod, info); >> + if (err < 0) >> + return err; >> + } >> + >> /* Arch-specific module finalizing. */ >> return module_finalize(info->hdr, info->sechdrs, mod); >> } >> @@ -3775,12 +3781,6 @@ static int load_module(struct load_info *info, const >> char __user *uargs, >> if (err < 0) >> goto coming_cleanup; >> >> - if (is_livepatch_module(mod)) { >> - err = copy_module_elf(mod, info); >> - if (err < 0) >> - goto sysfs_cleanup; >> - } >> - >> /* Get rid of temporary copy. */ >> free_copy(info); > >Hm, this is hopefully all right. We'd need to change the error handling a >bit. free_module_elf() is now called in free_module() and it should be >moved somewhere to load_module(). Probably under free_arch_cleanup label. >Although it would be much better to rework post_relocation() and handle it >right there. > >Something like > >- return module_finalize(info->hdr, info->sechdrs, mod); >+ err = module_finalize(info->hdr, info->sechdrs, mod); >+ if (err < 0) >+ free_module_elf(); >+ >+ return err; Ah yeah, I'll clean up the error handling. I'll re-submit this as a real patch after cleaning it up a bit and it could be added to this series after review. >> Thoughts? Does the fix make sense? > >I think so. Thanks Miroslav for the impromptu review :-) Jessica
On Fri, Oct 19, 2018 at 01:59:01PM +0200, Miroslav Benes wrote: > > Torsten, could you include the outcome to your patch set once we settle on > it? Thanks. Absolutely! Whether as patch 4/4 or on its own and I refer to it -- we'll figure it out. Torsten
On 18 October 2018 at 20:58, Jessica Yu <jeyu@kernel.org> wrote: > +++ Miroslav Benes [17/10/18 15:39 +0200]: > >> On Mon, 1 Oct 2018, Torsten Duwe wrote: >> >>> Based on ftrace with regs, do the usual thing. Also allocate a >>> task flag for whatever consistency handling will be used. >>> Watch out for interactions with the graph tracer. >> >> >> Similar to what Mark wrote about 2/4, I'd appreciate a better commit log. >> Could you explain traditional "what/why/how", please? >> >>> Signed-off-by: Torsten Duwe <duwe@suse.de> >>> >>> --- a/arch/arm64/Kconfig >>> +++ b/arch/arm64/Kconfig >>> @@ -119,6 +119,7 @@ config ARM64 >>> select HAVE_GENERIC_DMA_COHERENT >>> select HAVE_HW_BREAKPOINT if PERF_EVENTS >>> select HAVE_IRQ_TIME_ACCOUNTING >>> + select HAVE_LIVEPATCH >>> select HAVE_MEMBLOCK >>> select HAVE_MEMBLOCK_NODE_MAP if NUMA >>> select HAVE_NMI >>> @@ -1349,4 +1350,6 @@ if CRYPTO >>> source "arch/arm64/crypto/Kconfig" >>> endif >>> >>> +source "kernel/livepatch/Kconfig" >>> + >>> source "lib/Kconfig" >>> --- a/arch/arm64/include/asm/thread_info.h >>> +++ b/arch/arm64/include/asm/thread_info.h >>> @@ -76,6 +76,7 @@ void arch_release_task_struct(struct tas >>> #define TIF_FOREIGN_FPSTATE 3 /* CPU's FP state is not >>> current's */ >>> #define TIF_UPROBE 4 /* uprobe breakpoint or >>> singlestep */ >>> #define TIF_FSCHECK 5 /* Check FS is USER_DS on return >>> */ >>> +#define TIF_PATCH_PENDING 6 >>> #define TIF_NOHZ 7 >>> #define TIF_SYSCALL_TRACE 8 >>> #define TIF_SYSCALL_AUDIT 9 >>> @@ -94,6 +95,7 @@ void arch_release_task_struct(struct tas >>> #define _TIF_NEED_RESCHED (1 << TIF_NEED_RESCHED) >>> #define _TIF_NOTIFY_RESUME (1 << TIF_NOTIFY_RESUME) >>> #define _TIF_FOREIGN_FPSTATE (1 << TIF_FOREIGN_FPSTATE) >>> +#define _TIF_PATCH_PENDING (1 << TIF_PATCH_PENDING) >>> #define _TIF_NOHZ (1 << TIF_NOHZ) >>> #define _TIF_SYSCALL_TRACE (1 << TIF_SYSCALL_TRACE) >>> #define _TIF_SYSCALL_AUDIT (1 << TIF_SYSCALL_AUDIT) >>> @@ -106,7 +108,8 @@ void arch_release_task_struct(struct tas >>> >>> #define _TIF_WORK_MASK (_TIF_NEED_RESCHED | _TIF_SIGPENDING | \ >>> _TIF_NOTIFY_RESUME | >>> _TIF_FOREIGN_FPSTATE | \ >>> - _TIF_UPROBE | _TIF_FSCHECK) >>> + _TIF_UPROBE | _TIF_FSCHECK | \ >>> + _TIF_PATCH_PENDING) >> >> >> Could you add a note to the changelog what this means? My ability to read >> arm64 entry.S is very limited, but I can see that _TIF_WORK_MASK is >> process in a syscall exit and irq return paths. That's good. It is also >> called (via "b ret_to_user") in a couple of different places (el0_* >> labels). I guess those are returns from exception handling. A comment >> about it in the changelog would be appreciated. >> >>> #define _TIF_SYSCALL_WORK (_TIF_SYSCALL_TRACE | _TIF_SYSCALL_AUDIT >>> | \ >>> _TIF_SYSCALL_TRACEPOINT | _TIF_SECCOMP | >>> \ >>> --- /dev/null >>> +++ b/arch/arm64/include/asm/livepatch.h >>> @@ -0,0 +1,36 @@ >>> +/* SPDX-License-Identifier: GPL-2.0 >>> + * >>> + * livepatch.h - arm64-specific Kernel Live Patching Core >>> + * >>> + * Copyright (C) 2016,2018 SUSE >>> + * >>> + * This program is free software; you can redistribute it and/or >>> + * modify it under the terms of the GNU General Public License >>> + * as published by the Free Software Foundation; either version 2 >>> + * of the License, or (at your option) any later version. >>> + * >>> + * This program is distributed in the hope that it will be useful, >>> + * but WITHOUT ANY WARRANTY; without even the implied warranty of >>> + * MERCHANTABILITY or FITNESS FOR A PARTICULAR PURPOSE. See the >>> + * GNU General Public License for more details. >>> + * >>> + * You should have received a copy of the GNU General Public License >>> + * along with this program; if not, see <http://www.gnu.org/licenses/>. >>> + */ >>> +#ifndef _ASM_ARM64_LIVEPATCH_H >>> +#define _ASM_ARM64_LIVEPATCH_H >>> + >>> +#include <linux/module.h> >>> +#include <linux/ftrace.h> >> >> >> I think that only >> >> #include <asm/ptrace.h> >> >> is in fact needed, because of "struct pt_regs". >> >> >> Ad relocations. I checked that everything in struct mod_arch_specific >> stays after the module is load. Both core and init get SHF_ALLOC set >> (mod->arch.core.plt->sh_flags in module_frob_arch_sections(). It is >> important because apply_relocate_add() may use those sections >> through module_emit_plt_entry() call. > > > Yes, it looks like the needed .plt sections will remain in module > memory. However, I think I found a slight issue... :/ > > In module_frob_arch_sections(), mod->arch.core.plt is set to an offset > within info->sechdrs: > > if (!strcmp(secstrings + sechdrs[i].sh_name, ".plt")) > mod->arch.core.plt = sechdrs + i; > > sechdrs is from info->sechdrs, which is freed at the end of > load_module() via free_copy(). So although the relevant plt section(s) > are in module memory, mod->arch.core.plt will point to invalid memory > after info is freed. In other words, the section (.plt) *is* in memory > but the section header (struct elf64_shdr) containing section metadata > like sh_addr, sh_size etc., is not. > Just for my understanding: this only matters for live patching, right? The original PLT support was implemented to support loading modules outside of the -/+ 128 MB range of an arm64 relative branch/jump instruction, and was later enhanced [for the same reason] to support emitting a trampoline for the ftrace entrypoint. > But we have mod->klp_info->sechdrs (which is a copy of the section > headers) for this very reason. It is initialized only at the very end > of load_module(). I don't think copy_module_elf() is dependent on > anything, so it can just be moved earlier. > > Note that we cannot set mod->arch.core.plt to mod->klp_info->sechdrs + i > during module_frob_arch_sections() because it is still too early, none > of the module sections have been copied and none of their sh_addr's > have been set to their final locations as this is all happening before > move_module() is called. So we can use a module_finalize() function > for arm64 to switch the mod->arch.core.plt to use the klp_info section > headers. > > Maybe this will be more clear in the example fix below (completely > untested and unreviewed!): > > diff --git a/arch/arm64/include/asm/module.h > b/arch/arm64/include/asm/module.h > index 97d0ef12e2ff..150afc29171b 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..c23cef8f0165 100644 > --- a/arch/arm64/kernel/module-plts.c > +++ b/arch/arm64/kernel/module-plts.c > @@ -196,6 +196,21 @@ static unsigned int count_plts(Elf64_Sym *syms, > Elf64_Rela *rela, int num, > return ret; > } > > +int module_finalize(const Elf_Ehdr *hdr, const Elf_Shdr *sechdrs, > + struct module *mod) > +{ > + /* > + * Livepatch modules need to keep access to section headers to apply > + * relocations. Note mod->klp_info should have already been > initialized > + * and all section sh_addr's should have their final addresses by > the > + * time module_finalize() is called. > + */ > + if (is_livepatch_module(mod)) > + mod->arch.core.plt = mod->klp_info->sechdrs + > mod->arch.core.plt_shndx; > + > + return 0; > +} > + > int module_frob_arch_sections(Elf_Ehdr *ehdr, Elf_Shdr *sechdrs, > char *secstrings, struct module *mod) > { > @@ -210,11 +225,13 @@ 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")) > + 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) && > + mod->arch.init.plt_shndx = i; > + } else if (IS_ENABLED(CONFIG_DYNAMIC_FTRACE) && > !strcmp(secstrings + sechdrs[i].sh_name, > ".text.ftrace_trampoline")) > tramp = sechdrs + i; > diff --git a/kernel/module.c b/kernel/module.c > index 6746c85511fe..2fc4d74288dd 100644 > --- a/kernel/module.c > +++ b/kernel/module.c > @@ -3363,6 +3363,12 @@ static int post_relocation(struct module *mod, const > struct load_info *info) > /* Setup kallsyms-specific fields. */ > add_kallsyms(mod, info); > > + if (is_livepatch_module(mod)) { > + err = copy_module_elf(mod, info); > + if (err < 0) > + return err; > + } > + > /* Arch-specific module finalizing. */ > return module_finalize(info->hdr, info->sechdrs, mod); > } > @@ -3775,12 +3781,6 @@ static int load_module(struct load_info *info, const > char __user *uargs, > if (err < 0) > goto coming_cleanup; > > - if (is_livepatch_module(mod)) { > - err = copy_module_elf(mod, info); > - if (err < 0) > - goto sysfs_cleanup; > - } > - > /* Get rid of temporary copy. */ > free_copy(info); > > Thoughts? Does the fix make sense? > >> The last thing is count_plts() function called from >> module_frob_arch_sections(). It needed to be changed in 2016 as well. See >> Jessica's patch >> (20160713001113.GA30925@packer-debian-8-amd64.digitalocean.com). The logic >> in the function has changed since then. If I am not mistaken, >> count_plts() is fine as it is right now. It does not consider SHN_UNDEF >> anymore, it looks at the destination section (where a symbol should >> resolved to) only. >> >> Jessica, could you doublecheck, please? > > > Yes, I think count_plts() is fine now in this case (because it is > always true that sym->st_shndx != dstidx for SHN_LIVEPATCH symbols) > and my old patch from 2016 is no longer needed. > > Thanks, > > Jessica
> >If I am not mistaken, we do not care for arch.init.plt in livepatch. Is > >that correct? > > I do not believe patching of __init functions is supported (right?) So > we do not need to keep arch.init.plt alive post-module-load. I think we can do that. Theoretically. I'm not sure if it is actually useful. Module's init functions are called after the modules is patched, so there is no obstacle. But arch.init.plt would be useful only for the relocations of the patching module, right? Patching functions would not part of init section anyway, I think, so arch.init.plt is useless post-module-load. > >> int module_frob_arch_sections(Elf_Ehdr *ehdr, Elf_Shdr *sechdrs, > >> char *secstrings, struct module *mod) > >> { > >> @@ -210,11 +225,13 @@ 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")) > >> + 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) && > >> + mod->arch.init.plt_shndx = i; > > > >It is initialized here, but that's not necessarily a bad thing. > > I think I added this line for consistency, but I actually don't think > it is needed. We only would need to keep the section index for > arch.core.plt then. Yes. I'd welcome a comment somewhere in both cases. Either that we initialize it just for consistency, or that we don't, because it's not needed. Miroslav
> >> Ad relocations. I checked that everything in struct mod_arch_specific > >> stays after the module is load. Both core and init get SHF_ALLOC set > >> (mod->arch.core.plt->sh_flags in module_frob_arch_sections(). It is > >> important because apply_relocate_add() may use those sections > >> through module_emit_plt_entry() call. > > > > > > Yes, it looks like the needed .plt sections will remain in module > > memory. However, I think I found a slight issue... :/ > > > > In module_frob_arch_sections(), mod->arch.core.plt is set to an offset > > within info->sechdrs: > > > > if (!strcmp(secstrings + sechdrs[i].sh_name, ".plt")) > > mod->arch.core.plt = sechdrs + i; > > > > sechdrs is from info->sechdrs, which is freed at the end of > > load_module() via free_copy(). So although the relevant plt section(s) > > are in module memory, mod->arch.core.plt will point to invalid memory > > after info is freed. In other words, the section (.plt) *is* in memory > > but the section header (struct elf64_shdr) containing section metadata > > like sh_addr, sh_size etc., is not. > > > > Just for my understanding: this only matters for live patching, right? Yes. Live patching can do deferred relocations. When a module is loaded and livepatched, the relevant symbols in the patching module are resolved. We call apply_relocate_add(), which is arch-specific, and we must be sure that everything used by apply_relocate_add() is preserved in the patching module after it is loaded. > The original PLT support was implemented to support loading modules > outside of the -/+ 128 MB range of an arm64 relative branch/jump > instruction, and was later enhanced [for the same reason] to support > emitting a trampoline for the ftrace entrypoint. Regards, Miroslav
On 19 October 2018 at 23:21, Miroslav Benes <mbenes@suse.cz> wrote: > >> >> Ad relocations. I checked that everything in struct mod_arch_specific >> >> stays after the module is load. Both core and init get SHF_ALLOC set >> >> (mod->arch.core.plt->sh_flags in module_frob_arch_sections(). It is >> >> important because apply_relocate_add() may use those sections >> >> through module_emit_plt_entry() call. >> > >> > >> > Yes, it looks like the needed .plt sections will remain in module >> > memory. However, I think I found a slight issue... :/ >> > >> > In module_frob_arch_sections(), mod->arch.core.plt is set to an offset >> > within info->sechdrs: >> > >> > if (!strcmp(secstrings + sechdrs[i].sh_name, ".plt")) >> > mod->arch.core.plt = sechdrs + i; >> > >> > sechdrs is from info->sechdrs, which is freed at the end of >> > load_module() via free_copy(). So although the relevant plt section(s) >> > are in module memory, mod->arch.core.plt will point to invalid memory >> > after info is freed. In other words, the section (.plt) *is* in memory >> > but the section header (struct elf64_shdr) containing section metadata >> > like sh_addr, sh_size etc., is not. >> > >> >> Just for my understanding: this only matters for live patching, right? > > Yes. Live patching can do deferred relocations. When a module is loaded > and livepatched, the relevant symbols in the patching module are resolved. > We call apply_relocate_add(), which is arch-specific, and we must be sure > that everything used by apply_relocate_add() is preserved in the patching > module after it is loaded. > So I suppose this could get interesting in cases where modules are far away from the kernel (i.e., more than -/+ 128 MB). Fortunately, the modules themselves are always placed in a 128 MB window, but this window could be out of reach for branches into the kernel proper. If we find ourselves in the situation where we need to patch calls into the kernel proper to point into this module, this may get interesting, since the PLT entries *must* be allocated along with the module that contains the branch instruction, or the PLT entry itself may be out of range, defeating the purpose. >> The original PLT support was implemented to support loading modules >> outside of the -/+ 128 MB range of an arm64 relative branch/jump >> instruction, and was later enhanced [for the same reason] to support >> emitting a trampoline for the ftrace entrypoint. > > Regards, > Miroslav
On Sat, 20 Oct 2018, Ard Biesheuvel wrote: > On 19 October 2018 at 23:21, Miroslav Benes <mbenes@suse.cz> wrote: > > > >> >> Ad relocations. I checked that everything in struct mod_arch_specific > >> >> stays after the module is load. Both core and init get SHF_ALLOC set > >> >> (mod->arch.core.plt->sh_flags in module_frob_arch_sections(). It is > >> >> important because apply_relocate_add() may use those sections > >> >> through module_emit_plt_entry() call. > >> > > >> > > >> > Yes, it looks like the needed .plt sections will remain in module > >> > memory. However, I think I found a slight issue... :/ > >> > > >> > In module_frob_arch_sections(), mod->arch.core.plt is set to an offset > >> > within info->sechdrs: > >> > > >> > if (!strcmp(secstrings + sechdrs[i].sh_name, ".plt")) > >> > mod->arch.core.plt = sechdrs + i; > >> > > >> > sechdrs is from info->sechdrs, which is freed at the end of > >> > load_module() via free_copy(). So although the relevant plt section(s) > >> > are in module memory, mod->arch.core.plt will point to invalid memory > >> > after info is freed. In other words, the section (.plt) *is* in memory > >> > but the section header (struct elf64_shdr) containing section metadata > >> > like sh_addr, sh_size etc., is not. > >> > > >> > >> Just for my understanding: this only matters for live patching, right? > > > > Yes. Live patching can do deferred relocations. When a module is loaded > > and livepatched, the relevant symbols in the patching module are resolved. > > We call apply_relocate_add(), which is arch-specific, and we must be sure > > that everything used by apply_relocate_add() is preserved in the patching > > module after it is loaded. > > > > So I suppose this could get interesting in cases where modules are far > away from the kernel (i.e., more than -/+ 128 MB). Fortunately, the > modules themselves are always placed in a 128 MB window, but this > window could be out of reach for branches into the kernel proper. If > we find ourselves in the situation where we need to patch calls into > the kernel proper to point into this module, this may get interesting, > since the PLT entries *must* be allocated along with the module that > contains the branch instruction, or the PLT entry itself may be out of > range, defeating the purpose. Hm... Torsten, didn't you have to solve something similar in powerpc case? > >> The original PLT support was implemented to support loading modules > >> outside of the -/+ 128 MB range of an arm64 relative branch/jump > >> instruction, and was later enhanced [for the same reason] to support > >> emitting a trampoline for the ftrace entrypoint.
On Mon, Oct 22, 2018 at 02:53:10PM +0200, Miroslav Benes wrote: > On Sat, 20 Oct 2018, Ard Biesheuvel wrote: > > So I suppose this could get interesting in cases where modules are far > > away from the kernel (i.e., more than -/+ 128 MB). Fortunately, the > > modules themselves are always placed in a 128 MB window, but this > > window could be out of reach for branches into the kernel proper. If > > we find ourselves in the situation where we need to patch calls into > > the kernel proper to point into this module, this may get interesting, > > since the PLT entries *must* be allocated along with the module that > > contains the branch instruction, or the PLT entry itself may be out of > > range, defeating the purpose. > > Hm... Torsten, didn't you have to solve something similar in powerpc case? At address ranges: that was the TOC pointer issue, that is an array of addresses and other 64-bit constants with a reference to it kept in a register, whose value is local to any module. PPC64 needs 5 instructions to load a 64-bit value immediate, arm has the ldr_l macro. So in short: no. We were discussing a few nasty tricks to reconstruct the TOC value from the code addresses, but ended up with a solution that is clean in that respect. At PLTs: ppc64 uses, IIRC, a regular trampoline slot to get to all called functions, including ftrace and friends. So for e.g. live patch kernels the total number, as predetermined, is only incremented by 2. Only the trampoline _code_ for ftrace was modified. I assume with "branches" you mean "branch with link" a.k.a a subroutine call? All references to external symbols are allocated a PLT entry, right? ftrace / live patching calls are alway intercepted at the called function. ===========--------======--------- The interceptor then uses a special trampoline to go to ftrace_caller, which does the rest. Am I missing something? Torsten
--- a/arch/arm64/Kconfig +++ b/arch/arm64/Kconfig @@ -119,6 +119,7 @@ config ARM64 select HAVE_GENERIC_DMA_COHERENT select HAVE_HW_BREAKPOINT if PERF_EVENTS select HAVE_IRQ_TIME_ACCOUNTING + select HAVE_LIVEPATCH select HAVE_MEMBLOCK select HAVE_MEMBLOCK_NODE_MAP if NUMA select HAVE_NMI @@ -1349,4 +1350,6 @@ if CRYPTO source "arch/arm64/crypto/Kconfig" endif +source "kernel/livepatch/Kconfig" + source "lib/Kconfig" --- a/arch/arm64/include/asm/thread_info.h +++ b/arch/arm64/include/asm/thread_info.h @@ -76,6 +76,7 @@ void arch_release_task_struct(struct tas #define TIF_FOREIGN_FPSTATE 3 /* CPU's FP state is not current's */ #define TIF_UPROBE 4 /* uprobe breakpoint or singlestep */ #define TIF_FSCHECK 5 /* Check FS is USER_DS on return */ +#define TIF_PATCH_PENDING 6 #define TIF_NOHZ 7 #define TIF_SYSCALL_TRACE 8 #define TIF_SYSCALL_AUDIT 9 @@ -94,6 +95,7 @@ void arch_release_task_struct(struct tas #define _TIF_NEED_RESCHED (1 << TIF_NEED_RESCHED) #define _TIF_NOTIFY_RESUME (1 << TIF_NOTIFY_RESUME) #define _TIF_FOREIGN_FPSTATE (1 << TIF_FOREIGN_FPSTATE) +#define _TIF_PATCH_PENDING (1 << TIF_PATCH_PENDING) #define _TIF_NOHZ (1 << TIF_NOHZ) #define _TIF_SYSCALL_TRACE (1 << TIF_SYSCALL_TRACE) #define _TIF_SYSCALL_AUDIT (1 << TIF_SYSCALL_AUDIT) @@ -106,7 +108,8 @@ void arch_release_task_struct(struct tas #define _TIF_WORK_MASK (_TIF_NEED_RESCHED | _TIF_SIGPENDING | \ _TIF_NOTIFY_RESUME | _TIF_FOREIGN_FPSTATE | \ - _TIF_UPROBE | _TIF_FSCHECK) + _TIF_UPROBE | _TIF_FSCHECK | \ + _TIF_PATCH_PENDING) #define _TIF_SYSCALL_WORK (_TIF_SYSCALL_TRACE | _TIF_SYSCALL_AUDIT | \ _TIF_SYSCALL_TRACEPOINT | _TIF_SECCOMP | \ --- /dev/null +++ b/arch/arm64/include/asm/livepatch.h @@ -0,0 +1,36 @@ +/* SPDX-License-Identifier: GPL-2.0 + * + * livepatch.h - arm64-specific Kernel Live Patching Core + * + * Copyright (C) 2016,2018 SUSE + * + * This program is free software; you can redistribute it and/or + * modify it under the terms of the GNU General Public License + * as published by the Free Software Foundation; either version 2 + * of the License, or (at your option) any later version. + * + * This program is distributed in the hope that it will be useful, + * but WITHOUT ANY WARRANTY; without even the implied warranty of + * MERCHANTABILITY or FITNESS FOR A PARTICULAR PURPOSE. See the + * GNU General Public License for more details. + * + * You should have received a copy of the GNU General Public License + * along with this program; if not, see <http://www.gnu.org/licenses/>. + */ +#ifndef _ASM_ARM64_LIVEPATCH_H +#define _ASM_ARM64_LIVEPATCH_H + +#include <linux/module.h> +#include <linux/ftrace.h> + +static inline int klp_check_compiler_support(void) +{ + return 0; +} + +static inline void klp_arch_set_pc(struct pt_regs *regs, unsigned long ip) +{ + regs->pc = ip; +} + +#endif /* _ASM_ARM64_LIVEPATCH_H */ --- a/arch/arm64/kernel/entry-ftrace.S +++ b/arch/arm64/kernel/entry-ftrace.S @@ -209,6 +209,9 @@ ftrace_common: str x9, [sp, #S_LR] /* to pt_regs.r[30] */ /* The program counter just after the ftrace call site */ str lr, [sp, #S_PC] +#if defined(CONFIG_LIVEPATCH) && defined(CONFIG_FUNCTION_GRAPH_TRACER) + mov x28,lr /* remember old return address */ +#endif /* The stack pointer as it was on ftrace_caller entry... */ add x29, sp, #S_FRAME_SIZE str x29, [sp, #S_SP] @@ -224,6 +227,16 @@ ftrace_call: bl ftrace_stub +#if defined(CONFIG_LIVEPATCH) && defined(CONFIG_FUNCTION_GRAPH_TRACER) + /* Is the trace function a live patcher an has messed with + * the return address? + */ + ldr x9, [sp, #S_PC+16] + cmp x9, x28 /* compare with the value we remembered */ + /* to not call graph tracer's "call" mechanism twice! */ + b.ne ftrace_common_return +#endif + #ifdef CONFIG_FUNCTION_GRAPH_TRACER .global ftrace_graph_call ftrace_graph_call: // ftrace_graph_caller(); --- a/arch/arm64/kernel/signal.c +++ b/arch/arm64/kernel/signal.c @@ -29,6 +29,7 @@ #include <linux/sizes.h> #include <linux/string.h> #include <linux/tracehook.h> +#include <linux/livepatch.h> #include <linux/ratelimit.h> #include <linux/syscalls.h> @@ -934,6 +935,9 @@ asmlinkage void do_notify_resume(struct if (thread_flags & _TIF_UPROBE) uprobe_notify_resume(regs); + if (thread_flags & _TIF_PATCH_PENDING) + klp_update_patch_state(current); + if (thread_flags & _TIF_SIGPENDING) do_signal(regs);
Based on ftrace with regs, do the usual thing. Also allocate a task flag for whatever consistency handling will be used. Watch out for interactions with the graph tracer. Signed-off-by: Torsten Duwe <duwe@suse.de>