Message-ID: <20160818180313.GA8272@packer-debian-8-amd64.digitalocean.com>
Date:   Thu, 18 Aug 2016 14:03:13 -0400
From:   Jessica Yu <jeyu@...hat.com>
To:     Petr Mladek <pmladek@...e.com>
Cc:     Josh Poimboeuf <jpoimboe@...hat.com>,
        Miroslav Benes <mbenes@...e.cz>,
        Jiri Kosina <jikos@...nel.org>,
        Chris J Arges <chris.j.arges@...onical.com>,
        Eugene Shatokhin <eugene.shatokhin@...alab.ru>,
        live-patching@...r.kernel.org, x86@...nel.org,
        linux-doc@...r.kernel.org, linux-kernel@...r.kernel.org
Subject: Re: livepatch/x86: apply alternatives and paravirt patches after
 relocations

+++ 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 <jeyu@...hat.com>
>> ---
>>  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 <http://www.gnu.org/licenses/>.
>> + */
>> +
>> +#include <linux/module.h>
>> +#include <linux/kallsyms.h>
>> +#include <linux/livepatch.h>
>> +#include <asm/text-patching.h>
>> +
>> +/* 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

Powered by blists - more mailing lists