From mboxrd@z Thu Jan 1 00:00:00 1970 Return-Path: Received: (majordomo@vger.kernel.org) by vger.kernel.org via listexpand id S1753907AbcHSBMR (ORCPT ); Thu, 18 Aug 2016 21:12:17 -0400 Received: from mx1.redhat.com ([209.132.183.28]:33200 "EHLO mx1.redhat.com" rhost-flags-OK-OK-OK-OK) by vger.kernel.org with ESMTP id S1754202AbcHSBMP (ORCPT ); Thu, 18 Aug 2016 21:12:15 -0400 Date: Thu, 18 Aug 2016 14:03:13 -0400 From: Jessica Yu To: Petr Mladek Cc: Josh Poimboeuf , Miroslav Benes , Jiri Kosina , Chris J Arges , Eugene Shatokhin , live-patching@vger.kernel.org, x86@kernel.org, linux-doc@vger.kernel.org, linux-kernel@vger.kernel.org Subject: Re: livepatch/x86: apply alternatives and paravirt patches after relocations Message-ID: <20160818180313.GA8272@packer-debian-8-amd64.digitalocean.com> References: <1471481911-5003-1-git-send-email-jeyu@redhat.com> <1471481911-5003-3-git-send-email-jeyu@redhat.com> <20160818095153.GM13300@pathway.suse.cz> MIME-Version: 1.0 Content-Type: text/plain; charset=us-ascii; format=flowed Content-Disposition: inline In-Reply-To: <20160818095153.GM13300@pathway.suse.cz> X-OS: Linux eisen.io 3.16.0-4-amd64 x86_64 User-Agent: Mutt/1.5.23 (2014-03-12) X-Greylist: Sender IP whitelisted, not delayed by milter-greylist-4.5.16 (mx1.redhat.com [10.5.110.25]); Thu, 18 Aug 2016 18:03:15 +0000 (UTC) Sender: linux-kernel-owner@vger.kernel.org List-ID: X-Mailing-List: linux-kernel@vger.kernel.org +++ Petr Mladek [18/08/16 11:51 +0200]: >On Wed 2016-08-17 20:58:29, Jessica Yu wrote: >> Implement arch_klp_init_object_loaded() for x86, which applies >> alternatives/paravirt patches. This fixes the order in which relocations >> and alternatives/paravirt patches are applied. >> >> Previously, if a patch module had alternatives or paravirt patches, >> these were applied first by the module loader before livepatch can apply >> per-object relocations. The (buggy) sequence of events was: >> >> (1) Load patch module >> (2) Apply alternatives and paravirt patches to patch module >> * Note that these are applied to the new functions in the patch module >> (3) Apply per-object relocations to patch module when target module loads. >> * This clobbers what was written in step 2 >> >> This lead to crashes and corruption in general, since livepatch would >> overwrite or step on previously applied alternative/paravirt patches. >> The correct sequence of events should be: >> >> (1) Load patch module >> (2) Apply per-object relocations to patch module >> (3) Apply alternatives and paravirt patches to patch module >> >> This is fixed by delaying paravirt/alternatives patching until after >> relocations are applied. Any .altinstructions or .parainstructions >> sections are prefixed with ".klp.arch.${objname}" and applied in >> arch_klp_init_object_loaded(). >> >> Signed-off-by: Jessica Yu >> --- >> arch/x86/kernel/Makefile | 1 + >> arch/x86/kernel/livepatch.c | 65 +++++++++++++++++++++++++++++++++++++++++++++ >> 2 files changed, 66 insertions(+) >> create mode 100644 arch/x86/kernel/livepatch.c >> >> diff --git a/arch/x86/kernel/Makefile b/arch/x86/kernel/Makefile >> index d3f49c3..92fd50c 100644 >> --- a/arch/x86/kernel/Makefile >> +++ b/arch/x86/kernel/Makefile >> @@ -81,6 +81,7 @@ obj-$(CONFIG_X86_MPPARSE) += mpparse.o >> obj-y += apic/ >> obj-$(CONFIG_X86_REBOOTFIXUPS) += reboot_fixups_32.o >> obj-$(CONFIG_DYNAMIC_FTRACE) += ftrace.o >> +obj-$(CONFIG_LIVEPATCH) += livepatch.o >> obj-$(CONFIG_FUNCTION_GRAPH_TRACER) += ftrace.o >> obj-$(CONFIG_FTRACE_SYSCALLS) += ftrace.o >> obj-$(CONFIG_X86_TSC) += trace_clock.o >> diff --git a/arch/x86/kernel/livepatch.c b/arch/x86/kernel/livepatch.c >> new file mode 100644 >> index 0000000..e9d252d >> --- /dev/null >> +++ b/arch/x86/kernel/livepatch.c >> @@ -0,0 +1,65 @@ >> +/* >> + * livepatch.c - x86-specific Kernel Live Patching Core >> + * >> + * 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 . >> + */ >> + >> +#include >> +#include >> +#include >> +#include >> + >> +/* Apply per-object alternatives. Based on x86 module_finalize() */ >> +void arch_klp_init_object_loaded(struct klp_patch *patch, >> + struct klp_object *obj) >> +{ >> + int cnt; >> + struct klp_modinfo *info; >> + Elf_Shdr *s, *alt = NULL, *para = NULL; >> + void *aseg, *pseg; >> + const char *objname; >> + char sec_objname[MODULE_NAME_LEN]; >> + char secname[KSYM_NAME_LEN]; >> + >> + info = patch->mod->klp_info; >> + objname = obj->name ? obj->name : "vmlinux"; >> + >> + /* See livepatch core code for BUILD_BUG_ON() explanation */ >> + BUILD_BUG_ON(MODULE_NAME_LEN < 56 || KSYM_NAME_LEN != 128); >> + >> + for (s = info->sechdrs; s < info->sechdrs + info->hdr.e_shnum; s++) { >> + /* Apply per-object .klp.arch sections */ >> + cnt = sscanf(info->secstrings + s->sh_name, >> + ".klp.arch.%55[^.].%127s", >> + sec_objname, secname); >> + if (cnt != 2) >> + continue; >> + if (strcmp(sec_objname, objname)) >> + continue; >> + if (!strcmp(".altinstructions", secname)) > >The previous version of the patch compared against "altinstructions" >(without the dot). I admit that I haven't tested it but the dot >looks suspicious here. Good eye, I should have explained why the dot is needed in the strcmp.. So, the new documentation states that any arch-specific sections to be applied by livepatch are to be prefixed with the string ".klp.arch.$objname.", note the required dot at the end of this prefix. So for example, if we have a .parainstructions section with a patch for the kvm module, the prefixed section name would look like: .klp.arch.kvm..parainstructions ^ prefix ^^ original name ^ That extra dot looks weird, but it is needed when we have section names like "__ftr_fixup" on powerpc. Without the extra dot at the end of ".klp.arch.$objname." We'd get names like ".klp.arch.$objname__ftr_fixup", and we wouldn't be able to tell where the objname ends and where the section name begins. But with ".klp.arch.$objname.__ftr_fixup", we have a hard delimeter and know that after the dot after $objname comes the original section name. Hope that helps! Jessica