LinuxLists.cc - [PATCHv4 0/3] Kernel Live Patching

[permalink] [raw]

Subject: [PATCHv4 3/3] samples: add sample live patching module

Add a sample live patching module.

Signed-off-by: Seth Jennings <[email protected]>
---
MAINTAINERS | 1 +
samples/Kconfig | 7 +++
samples/livepatch/Makefile | 1 +
samples/livepatch/livepatch-sample.c | 87 ++++++++++++++++++++++++++++++++++++
4 files changed, 96 insertions(+)
create mode 100644 samples/livepatch/Makefile
create mode 100644 samples/livepatch/livepatch-sample.c

diff --git a/MAINTAINERS b/MAINTAINERS
index 7985293..9d3f9d9 100644
--- a/MAINTAINERS
+++ b/MAINTAINERS
@@ -5726,6 +5726,7 @@ F: include/linux/livepatch.h
F: arch/x86/include/asm/livepatch.h
F: arch/x86/kernel/livepatch.c
F: Documentation/ABI/testing/sysfs-kernel-livepatch
+F: samples/livepatch/
L: [email protected]

LLC (802.2)
diff --git a/samples/Kconfig b/samples/Kconfig
index 6181c2c..0aed20d 100644
--- a/samples/Kconfig
+++ b/samples/Kconfig
@@ -63,4 +63,11 @@ config SAMPLE_RPMSG_CLIENT
to communicate with an AMP-configured remote processor over
the rpmsg bus.

+config SAMPLE_LIVE_PATCHING
+ tristate "Build live patching sample -- loadable modules only"
+ depends on LIVE_PATCHING && m
+ help
+ Builds a sample live patch that replaces the procfs handler
+ for /proc/cmdline to print "this has been live patched".
+
endif # SAMPLES
diff --git a/samples/livepatch/Makefile b/samples/livepatch/Makefile
new file mode 100644
index 0000000..7f1cdc1
--- /dev/null
+++ b/samples/livepatch/Makefile
@@ -0,0 +1 @@
+obj-$(CONFIG_SAMPLE_LIVE_PATCHING) += livepatch-sample.o
diff --git a/samples/livepatch/livepatch-sample.c b/samples/livepatch/livepatch-sample.c
new file mode 100644
index 0000000..21f159d
--- /dev/null
+++ b/samples/livepatch/livepatch-sample.c
@@ -0,0 +1,87 @@
+/*
+ * livepatch-sample.c - Kernel Live Patching Sample Module
+ *
+ * Copyright (C) 2014 Seth Jennings <[email protected]>
+ *
+ * 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/kernel.h>
+#include <linux/livepatch.h>
+
+/*
+ * This (dumb) live patch overrides the function that prints the
+ * kernel boot cmdline when /proc/cmdline is read.
+ *
+ * Example:
+ * $ cat /proc/cmdline
+ * <your cmdline>
+ * $ insmod livepatch-sample.ko
+ * $ cat /proc/cmdline
+ * this has been live patched
+ * $ echo 0 > /sys/kernel/livepatch/klp_sample/enabled
+ * <your cmdline>
+ */
+
+#include <linux/seq_file.h>
+static int livepatch_cmdline_proc_show(struct seq_file *m, void *v)
+{
+ seq_printf(m, "%s\n", "this has been live patched");
+ return 0;
+}
+
+static struct klp_func funcs[] = {
+ {
+ .old_name = "cmdline_proc_show",
+ .new_func = livepatch_cmdline_proc_show,
+ }, { }
+};
+
+static struct klp_object objs[] = {
+ {
+ /* name being NULL means vmlinux */
+ .funcs = funcs,
+ }, { }
+};
+
+static struct klp_patch patch = {
+ .mod = THIS_MODULE,
+ .objs = objs,
+};
+
+static int livepatch_init(void)
+{
+ int ret;
+
+ ret = klp_register_patch(&patch);
+ if (ret)
+ return ret;
+ ret = klp_enable_patch(&patch);
+ if (ret) {
+ WARN_ON(klp_unregister_patch(&patch));
+ return ret;
+ }
+ return 0;
+}
+
+static void livepatch_exit(void)
+{
+ WARN_ON(klp_disable_patch(&patch));
+ WARN_ON(klp_unregister_patch(&patch));
+}
+
+module_init(livepatch_init);
+module_exit(livepatch_exit);
+MODULE_LICENSE("GPL");
--
1.9.3

2014-11-25 17:16:14

[permalink] [raw]

Subject: [PATCHv4 2/3] kernel: add support for live patching

This commit introduces code for the live patching core. It implements
an ftrace-based mechanism and kernel interface for doing live patching
of kernel and kernel module functions.

It represents the greatest common functionality set between kpatch and
kgraft and can accept patches built using either method.

This first version does not implement any consistency mechanism that
ensures that old and new code do not run together. In practice, ~90% of
CVEs are safe to apply in this way, since they simply add a conditional
check. However, any function change that can not execute safely with
the old version of the function can _not_ be safely applied in this
version.

Signed-off-by: Seth Jennings <[email protected]>
---
Documentation/ABI/testing/sysfs-kernel-livepatch | 44 ++
MAINTAINERS | 13 +
arch/x86/Kconfig | 3 +
arch/x86/include/asm/livepatch.h | 40 ++
arch/x86/kernel/Makefile | 1 +
arch/x86/kernel/livepatch.c | 74 +++
include/linux/livepatch.h | 121 ++++
kernel/Makefile | 1 +
kernel/livepatch/Kconfig | 18 +
kernel/livepatch/Makefile | 3 +
kernel/livepatch/core.c | 807 +++++++++++++++++++++++
11 files changed, 1125 insertions(+)
create mode 100644 Documentation/ABI/testing/sysfs-kernel-livepatch
create mode 100644 arch/x86/include/asm/livepatch.h
create mode 100644 arch/x86/kernel/livepatch.c
create mode 100644 include/linux/livepatch.h
create mode 100644 kernel/livepatch/Kconfig
create mode 100644 kernel/livepatch/Makefile
create mode 100644 kernel/livepatch/core.c

diff --git a/Documentation/ABI/testing/sysfs-kernel-livepatch b/Documentation/ABI/testing/sysfs-kernel-livepatch
new file mode 100644
index 0000000..5bf42a8
--- /dev/null
+++ b/Documentation/ABI/testing/sysfs-kernel-livepatch
@@ -0,0 +1,44 @@
+What: /sys/kernel/livepatch
+Date: Nov 2014
+KernelVersion: 3.19.0
+Contact: [email protected]
+Description:
+ Interface for kernel live patching
+
+ The /sys/kernel/livepatch directory contains subdirectories for
+ each loaded live patch module.
+
+What: /sys/kernel/livepatch/<patch>
+Date: Nov 2014
+KernelVersion: 3.19.0
+Contact: [email protected]
+Description:
+ The patch directory contains subdirectories for each kernel
+ object (vmlinux or a module) in which it patched functions.
+
+What: /sys/kernel/livepatch/<patch>/enabled
+Date: Nov 2014
+KernelVersion: 3.19.0
+Contact: [email protected]
+Description:
+ A writable attribute that indicates whether the patched
+ code is currently applied. Writing 0 will disable the patch
+ while writing 1 will re-enable the patch.
+
+What: /sys/kernel/livepatch/<patch>/<object>
+Date: Nov 2014
+KernelVersion: 3.19.0
+Contact: [email protected]
+Description:
+ The object directory contains subdirectories for each function
+ that is patched within the object.
+
+What: /sys/kernel/livepatch/<patch>/<object>/<function>
+Date: Nov 2014
+KernelVersion: 3.19.0
+Contact: [email protected]
+Description:
+ The function directory contains attributes regarding the
+ properties and state of the patched function.
+
+ There are currently no such attributes.
diff --git a/MAINTAINERS b/MAINTAINERS
index 4861577..7985293 100644
--- a/MAINTAINERS
+++ b/MAINTAINERS
@@ -5715,6 +5715,19 @@ F: Documentation/misc-devices/lis3lv02d
F: drivers/misc/lis3lv02d/
F: drivers/platform/x86/hp_accel.c

+LIVE PATCHING
+M: Josh Poimboeuf <[email protected]>
+M: Seth Jennings <[email protected]>
+M: Jiri Kosina <[email protected]>
+M: Vojtech Pavlik <[email protected]>
+S: Maintained
+F: kernel/livepatch/
+F: include/linux/livepatch.h
+F: arch/x86/include/asm/livepatch.h
+F: arch/x86/kernel/livepatch.c
+F: Documentation/ABI/testing/sysfs-kernel-livepatch
+L: [email protected]
+
LLC (802.2)
M: Arnaldo Carvalho de Melo <[email protected]>
S: Maintained
diff --git a/arch/x86/Kconfig b/arch/x86/Kconfig
index ec21dfd..78715fd 100644
--- a/arch/x86/Kconfig
+++ b/arch/x86/Kconfig
@@ -17,6 +17,7 @@ config X86_64
depends on 64BIT
select X86_DEV_DMA_OPS
select ARCH_USE_CMPXCHG_LOCKREF
+ select ARCH_HAVE_LIVE_PATCHING

### Arch settings
config X86
@@ -1991,6 +1992,8 @@ config CMDLINE_OVERRIDE
This is used to work around broken boot loaders. This should
be set to 'N' under normal conditions.

+source "kernel/livepatch/Kconfig"
+
endmenu

config ARCH_ENABLE_MEMORY_HOTPLUG
diff --git a/arch/x86/include/asm/livepatch.h b/arch/x86/include/asm/livepatch.h
new file mode 100644
index 0000000..38fa496
--- /dev/null
+++ b/arch/x86/include/asm/livepatch.h
@@ -0,0 +1,40 @@
+/*
+ * livepatch.h - x86-specific Kernel Live Patching Core
+ *
+ * Copyright (C) 2014 Seth Jennings <[email protected]>
+ *
+ * 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_X86_LIVEPATCH_H
+#define _ASM_X86_LIVEPATCH_H
+
+#include <linux/module.h>
+
+#ifdef CONFIG_LIVE_PATCHING
+#ifndef CC_USING_FENTRY
+#error Your compiler must support -mfentry for live patching to work
+#endif
+extern int klp_write_module_reloc(struct module *mod, unsigned long type,
+ unsigned long loc, unsigned long value);
+
+#else
+static inline int klp_write_module_reloc(struct module *mod, unsigned long type,
+ unsigned long loc, unsigned long value)
+{
+ return -ENOSYS;
+}
+#endif
+
+#endif /* _ASM_X86_LIVEPATCH_H */
diff --git a/arch/x86/kernel/Makefile b/arch/x86/kernel/Makefile
index 5d4502c..316b34e 100644
--- a/arch/x86/kernel/Makefile
+++ b/arch/x86/kernel/Makefile
@@ -63,6 +63,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_LIVE_PATCHING) += 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..777a4a4
--- /dev/null
+++ b/arch/x86/kernel/livepatch.c
@@ -0,0 +1,74 @@
+/*
+ * livepatch.c - x86-specific Kernel Live Patching Core
+ *
+ * Copyright (C) 2014 Seth Jennings <[email protected]>
+ *
+ * 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/uaccess.h>
+#include <asm/cacheflush.h>
+#include <asm/page_types.h>
+
+int klp_write_module_reloc(struct module *mod, unsigned long type,
+ unsigned long loc, unsigned long value)
+{
+ int ret, readonly, numpages, size = 4;
+ unsigned long val;
+ unsigned long core = (unsigned long)mod->module_core;
+ unsigned long core_ro_size = mod->core_ro_size;
+ unsigned long core_size = mod->core_size;
+
+ switch (type) {
+ case R_X86_64_NONE:
+ return 0;
+ case R_X86_64_64:
+ val = value;
+ size = 8;
+ break;
+ case R_X86_64_32:
+ val = (u32)value;
+ break;
+ case R_X86_64_32S:
+ val = (s32)value;
+ break;
+ case R_X86_64_PC32:
+ val = (u32)(value - loc);
+ break;
+ default:
+ /* unsupported relocation type */
+ return -EINVAL;
+ }
+
+ if (loc >= core && loc < core + core_ro_size)
+ readonly = 1;
+ else if (loc >= core + core_ro_size && loc < core + core_size)
+ readonly = 0;
+ else
+ /* loc not in expected range */
+ return -EINVAL;
+
+ numpages = ((loc & PAGE_MASK) == ((loc + size) & PAGE_MASK)) ? 1 : 2;
+
+ if (readonly)
+ set_memory_rw(loc & PAGE_MASK, numpages);
+
+ ret = probe_kernel_write((void *)loc, &val, size);
+
+ if (readonly)
+ set_memory_ro(loc & PAGE_MASK, numpages);
+
+ return ret;
+}
diff --git a/include/linux/livepatch.h b/include/linux/livepatch.h
new file mode 100644
index 0000000..4e01b59
--- /dev/null
+++ b/include/linux/livepatch.h
@@ -0,0 +1,121 @@
+/*
+ * livepatch.h - Kernel Live Patching Core
+ *
+ * Copyright (C) 2014 Seth Jennings <[email protected]>
+ *
+ * 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 _LINUX_LIVEPATCH_H_
+#define _LINUX_LIVEPATCH_H_
+
+#include <linux/module.h>
+#include <linux/ftrace.h>
+
+#if IS_ENABLED(CONFIG_LIVE_PATCHING)
+
+#include <asm/livepatch.h>
+
+enum klp_state {
+ KLP_DISABLED,
+ KLP_ENABLED
+};
+
+/**
+ * struct klp_func - function structure for live patching
+ * @old_name: name of the function to be patched
+ * @new_func: pointer to the patched function code
+ * @old_addr: a hint conveying at what address the old function
+ * can be found (optional, vmlinux patches only)
+ */
+struct klp_func {
+ /* external */
+ const char *old_name;
+ void *new_func;
+ /*
+ * The old_addr field is optional and can be used to resolve
+ * duplicate symbol names in the vmlinux object. If this
+ * information is not present, the symbol is located by name
+ * with kallsyms. If the name is not unique and old_addr is
+ * not provided, the patch application fails as there is no
+ * way to resolve the ambiguity.
+ */
+ unsigned long old_addr;
+
+ /* internal */
+ struct kobject kobj;
+ struct ftrace_ops *fops;
+ enum klp_state state;
+};
+
+/**
+ * struct klp_reloc - relocation structure for live patching
+ * @dest address where the relocation will be written
+ * @src address of the referenced symbol (optional,
+ * vmlinux patches only)
+ * @type ELF relocation type
+ * @name name of the referenced symbol (for lookup/verification)
+ * @addend offset from the referenced symbol
+ * @external symbol is either exported or within the live patch module itself
+ */
+struct klp_reloc {
+ unsigned long dest;
+ unsigned long src;
+ unsigned long type;
+ const char *name;
+ int addend;
+ int external;
+};
+
+/* struct klp_object - kernel object structure for live patching
+ * @name module name (or NULL for vmlinux)
+ * @relocs relocation entries to be applied at load time
+ * @funcs function entries for functions to be patched in the object
+ */
+struct klp_object {
+ /* external */
+ const char *name;
+ struct klp_reloc *relocs;
+ struct klp_func *funcs;
+
+ /* internal */
+ struct kobject *kobj;
+ struct module *mod;
+ enum klp_state state;
+};
+
+/**
+ * struct klp_patch - patch structure for live patching
+ * @mod reference to the live patch module
+ * @objs object entries for kernel objects to be patched
+ */
+struct klp_patch {
+ /* external */
+ struct module *mod;
+ struct klp_object *objs;
+
+ /* internal */
+ struct list_head list;
+ struct kobject kobj;
+ enum klp_state state;
+};
+
+extern int klp_register_patch(struct klp_patch *);
+extern int klp_unregister_patch(struct klp_patch *);
+extern int klp_enable_patch(struct klp_patch *);
+extern int klp_disable_patch(struct klp_patch *);
+
+#endif /* CONFIG_LIVE_PATCHING */
+
+#endif /* _LINUX_LIVEPATCH_H_ */
diff --git a/kernel/Makefile b/kernel/Makefile
index a59481a..616994f 100644
--- a/kernel/Makefile
+++ b/kernel/Makefile
@@ -26,6 +26,7 @@ obj-y += power/
obj-y += printk/
obj-y += irq/
obj-y += rcu/
+obj-y += livepatch/

obj-$(CONFIG_CHECKPOINT_RESTORE) += kcmp.o
obj-$(CONFIG_FREEZER) += freezer.o
diff --git a/kernel/livepatch/Kconfig b/kernel/livepatch/Kconfig
new file mode 100644
index 0000000..96da00f
--- /dev/null
+++ b/kernel/livepatch/Kconfig
@@ -0,0 +1,18 @@
+config ARCH_HAVE_LIVE_PATCHING
+ boolean
+ help
+ Arch supports kernel live patching
+
+config LIVE_PATCHING
+ boolean "Kernel Live Patching"
+ depends on DYNAMIC_FTRACE_WITH_REGS
+ depends on MODULES
+ depends on SYSFS
+ depends on KALLSYMS_ALL
+ depends on ARCH_HAVE_LIVE_PATCHING
+ help
+ Say Y here if you want to support kernel live patching.
+ This option has no runtime impact until a kernel "patch"
+ module uses the interface provided by this option to register
+ a patch, causing calls to patched functions to be redirected
+ to new function code contained in the patch module.
diff --git a/kernel/livepatch/Makefile b/kernel/livepatch/Makefile
new file mode 100644
index 0000000..7c1f008
--- /dev/null
+++ b/kernel/livepatch/Makefile
@@ -0,0 +1,3 @@
+obj-$(CONFIG_LIVE_PATCHING) += livepatch.o
+
+livepatch-objs := core.o
diff --git a/kernel/livepatch/core.c b/kernel/livepatch/core.c
new file mode 100644
index 0000000..8e2e8cd
--- /dev/null
+++ b/kernel/livepatch/core.c
@@ -0,0 +1,807 @@
+/*
+ * core.c - Kernel Live Patching Core
+ *
+ * Copyright (C) 2014 Seth Jennings <[email protected]>
+ *
+ * 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/>.
+ */
+
+#define pr_fmt(fmt) KBUILD_MODNAME ": " fmt
+
+#include <linux/module.h>
+#include <linux/kernel.h>
+#include <linux/mutex.h>
+#include <linux/slab.h>
+#include <linux/ftrace.h>
+#include <linux/list.h>
+#include <linux/kallsyms.h>
+#include <linux/livepatch.h>
+
+/*
+ * The klp_mutex protects the klp_patches list and state transitions of any
+ * structure reachable from the patches list. References to any structure must
+ * be obtained under mutex protection.
+ */
+
+static DEFINE_MUTEX(klp_mutex);
+static LIST_HEAD(klp_patches);
+
+static struct kobject *klp_root_kobj;
+
+/* sets obj->mod if object is not vmlinux and module is found */
+static bool klp_find_object_module(struct klp_object *obj)
+{
+ if (!obj->name)
+ return 1;
+
+ mutex_lock(&module_mutex);
+ /*
+ * We don't need to take a reference on the module here because we have
+ * the klp_mutex, which is also taken by the module notifier. This
+ * prevents any module from unloading until we release the klp_mutex.
+ */
+ obj->mod = find_module(obj->name);
+ mutex_unlock(&module_mutex);
+
+ return !!obj->mod;
+}
+
+struct klp_find_arg {
+ const char *objname;
+ const char *name;
+ unsigned long addr;
+ /*
+ * If count == 0, the symbol was not found. If count == 1, a unique
+ * match was found and addr is set. If count > 1, there is
+ * unresolvable ambiguity among "count" number of symbols with the same
+ * name in the same object.
+ */
+ unsigned long count;
+};
+
+static int klp_find_callback(void *data, const char *name,
+ struct module *mod, unsigned long addr)
+{
+ struct klp_find_arg *args = data;
+
+ if ((mod && !args->objname) || (!mod && args->objname))
+ return 0;
+
+ if (strcmp(args->name, name))
+ return 0;
+
+ if (args->objname && strcmp(args->objname, mod->name))
+ return 0;
+
+ /*
+ * args->addr might be overwritten if another match is found
+ * but klp_find_symbol() handles this and only returns the
+ * addr if count == 1.
+ */
+ args->addr = addr;
+ args->count++;
+
+ return 0;
+}
+
+static int klp_find_symbol(const char *objname, const char *name,
+ unsigned long *addr)
+{
+ struct klp_find_arg args = {
+ .objname = objname,
+ .name = name,
+ .addr = 0,
+ .count = 0
+ };
+
+ kallsyms_on_each_symbol(klp_find_callback, &args);
+
+ if (args.count == 0)
+ pr_err("symbol '%s' not found in symbol table\n", name);
+ else if (args.count > 1)
+ pr_err("unresolvable ambiguity (%lu matches) on symbol '%s' in object '%s'\n",
+ args.count, name, objname);
+ else {
+ *addr = args.addr;
+ return 0;
+ }
+
+ *addr = 0;
+ return -EINVAL;
+}
+
+struct klp_verify_args {
+ const char *name;
+ const unsigned long addr;
+};
+
+static int klp_verify_callback(void *data, const char *name,
+ struct module *mod, unsigned long addr)
+{
+ struct klp_verify_args *args = data;
+
+ if (!mod &&
+ !strcmp(args->name, name) &&
+ args->addr == addr)
+ return 1;
+ return 0;
+}
+
+static int klp_verify_vmlinux_symbol(const char *name, unsigned long addr)
+{
+ struct klp_verify_args args = {
+ .name = name,
+ .addr = addr,
+ };
+
+ if (kallsyms_on_each_symbol(klp_verify_callback, &args))
+ return 0;
+ pr_err("symbol '%s' not found at specified address 0x%016lx, kernel mismatch?",
+ name, addr);
+ return -EINVAL;
+}
+
+static int klp_find_verify_func_addr(struct klp_func *func, const char *objname)
+{
+ int ret;
+
+#if defined(CONFIG_RANDOMIZE_BASE)
+ /* KASLR is enabled, disregard old_addr from user */
+ func->old_addr = 0;
+#endif
+
+ if (func->old_addr && objname) {
+ pr_err("old address specified for module symbol\n");
+ return -EINVAL;
+ }
+
+ if (func->old_addr)
+ ret = klp_verify_vmlinux_symbol(func->old_name,
+ func->old_addr);
+ else
+ ret = klp_find_symbol(objname, func->old_name,
+ &func->old_addr);
+
+ return ret;
+}
+
+/*
+ * external symbols are located outside the parent object (where the parent
+ * object is either vmlinux or the kmod being patched).
+ */
+static int klp_find_external_symbol(struct module *pmod, const char *name,
+ unsigned long *addr)
+{
+ const struct kernel_symbol *sym;
+
+ /* first, check if it's an exported symbol */
+ preempt_disable();
+ sym = find_symbol(name, NULL, NULL, true, true);
+ preempt_enable();
+ if (sym) {
+ *addr = sym->value;
+ return 0;
+ }
+
+ /* otherwise check if it's in another .o within the patch module */
+ return klp_find_symbol(pmod->name, name, addr);
+}
+
+static int klp_write_object_relocations(struct module *pmod,
+ struct klp_object *obj)
+{
+ int ret;
+ struct klp_reloc *reloc;
+
+ for (reloc = obj->relocs; reloc->name; reloc++) {
+ if (!obj->name) {
+ ret = klp_verify_vmlinux_symbol(reloc->name,
+ reloc->src);
+ if (ret)
+ return ret;
+ } else {
+ /* module, reloc->src needs to be discovered */
+ if (reloc->external)
+ ret = klp_find_external_symbol(pmod,
+ reloc->name,
+ &reloc->src);
+ else
+ ret = klp_find_symbol(obj->mod->name,
+ reloc->name,
+ &reloc->src);
+ if (ret)
+ return ret;
+ }
+ ret = klp_write_module_reloc(pmod, reloc->type, reloc->dest,
+ reloc->src + reloc->addend);
+ if (ret) {
+ pr_err("relocation failed for symbol '%s' at 0x%016lx (%d)\n",
+ reloc->name, reloc->src, ret);
+ return ret;
+ }
+ }
+
+ return 0;
+}
+
+static void klp_ftrace_handler(unsigned long ip, unsigned long parent_ip,
+ struct ftrace_ops *ops, struct pt_regs *regs)
+{
+ struct klp_func *func = ops->private;
+
+ regs->ip = (unsigned long)func->new_func;
+}
+
+static int klp_enable_func(struct klp_func *func)
+{
+ int ret;
+
+ if (WARN_ON(!func->old_addr))
+ return -EINVAL;
+
+ if (WARN_ON(func->state != KLP_DISABLED))
+ return -EINVAL;
+
+ ret = ftrace_set_filter_ip(func->fops, func->old_addr, 0, 0);
+ if (ret) {
+ pr_err("failed to set ftrace filter for function '%s' (%d)\n",
+ func->old_name, ret);
+ return ret;
+ }
+ ret = register_ftrace_function(func->fops);
+ if (ret) {
+ pr_err("failed to register ftrace handler for function '%s' (%d)\n",
+ func->old_name, ret);
+ ftrace_set_filter_ip(func->fops, func->old_addr, 1, 0);
+ } else
+ func->state = KLP_ENABLED;
+
+ return ret;
+}
+
+static int klp_disable_func(struct klp_func *func)
+{
+ int ret;
+
+ if (WARN_ON(func->state != KLP_ENABLED))
+ return -EINVAL;
+
+ if (!func->old_addr)
+ /* parent object is not loaded */
+ return 0;
+ ret = unregister_ftrace_function(func->fops);
+ if (ret) {
+ pr_err("failed to unregister ftrace handler for function '%s' (%d)\n",
+ func->old_name, ret);
+ return ret;
+ }
+ ret = ftrace_set_filter_ip(func->fops, func->old_addr, 1, 0);
+ if (ret)
+ pr_warn("function unregister succeeded but failed to clear the filter\n");
+ func->state = KLP_DISABLED;
+
+ return 0;
+}
+
+static int klp_disable_object(struct klp_object *obj)
+{
+ struct klp_func *func;
+ int ret;
+
+ for (func = obj->funcs; func->old_name; func++) {
+ if (func->state != KLP_ENABLED)
+ continue;
+ ret = klp_disable_func(func);
+ if (ret)
+ return ret;
+ if (obj->name)
+ func->old_addr = 0;
+ }
+ obj->state = KLP_DISABLED;
+
+ return 0;
+}
+
+static int klp_enable_object(struct module *pmod, struct klp_object *obj)
+{
+ struct klp_func *func;
+ int ret;
+
+ if (WARN_ON(!obj->mod && obj->name))
+ return -EINVAL;
+
+ if (obj->relocs) {
+ ret = klp_write_object_relocations(pmod, obj);
+ if (ret)
+ goto unregister;
+ }
+
+ for (func = obj->funcs; func->old_name; func++) {
+ ret = klp_find_verify_func_addr(func, obj->name);
+ if (ret)
+ goto unregister;
+
+ ret = klp_enable_func(func);
+ if (ret)
+ goto unregister;
+ }
+ obj->state = KLP_ENABLED;
+
+ return 0;
+unregister:
+ WARN_ON(klp_disable_object(obj));
+ return ret;
+}
+
+static int __klp_disable_patch(struct klp_patch *patch)
+{
+ struct klp_object *obj;
+ int ret;
+
+ pr_notice("disabling patch '%s'\n", patch->mod->name);
+
+ for (obj = patch->objs; obj->funcs; obj++) {
+ if (obj->state != KLP_ENABLED)
+ continue;
+ ret = klp_disable_object(obj);
+ if (ret)
+ return ret;
+ }
+ patch->state = KLP_DISABLED;
+
+ return 0;
+}
+
+/**
+ * klp_disable_patch() - disables a registered patch
+ * @patch: The registered, enabled patch to be disabled
+ *
+ * Unregisters the patched functions from ftrace.
+ *
+ * Return: 0 on success, otherwise error
+ */
+int klp_disable_patch(struct klp_patch *patch)
+{
+ int ret;
+
+ mutex_lock(&klp_mutex);
+ if (patch->state == KLP_ENABLED) {
+ ret = -EINVAL;
+ goto out;
+ }
+ ret = __klp_disable_patch(patch);
+out:
+ mutex_unlock(&klp_mutex);
+ return ret;
+}
+EXPORT_SYMBOL_GPL(klp_disable_patch);
+
+static int __klp_enable_patch(struct klp_patch *patch)
+{
+ struct klp_object *obj;
+ int ret;
+
+ if (WARN_ON(patch->state != KLP_DISABLED))
+ return -EINVAL;
+
+ pr_notice_once("tainting kernel with TAINT_LIVEPATCH\n");
+ add_taint(TAINT_LIVEPATCH, LOCKDEP_STILL_OK);
+
+ pr_notice("enabling patch '%s'\n", patch->mod->name);
+
+ for (obj = patch->objs; obj->funcs; obj++) {
+ if (!klp_find_object_module(obj))
+ continue;
+ ret = klp_enable_object(patch->mod, obj);
+ if (ret)
+ goto unregister;
+ }
+ patch->state = KLP_ENABLED;
+ return 0;
+
+unregister:
+ WARN_ON(__klp_disable_patch(patch));
+ return ret;
+}
+
+/**
+ * klp_enable_patch() - enables a registered patch
+ * @patch: The registered, disabled patch to be enabled
+ *
+ * Performs the needed symbol lookups and code relocations,
+ * then registers the patched functions with ftrace.
+ *
+ * Return: 0 on success, otherwise error
+ */
+int klp_enable_patch(struct klp_patch *patch)
+{
+ int ret;
+
+ mutex_lock(&klp_mutex);
+ ret = __klp_enable_patch(patch);
+ mutex_unlock(&klp_mutex);
+
+ return ret;
+}
+EXPORT_SYMBOL_GPL(klp_enable_patch);
+
+static void klp_module_notify_coming(struct module *pmod,
+ struct klp_object *obj)
+{
+ struct module *mod = obj->mod;
+ int ret;
+
+ pr_notice("applying patch '%s' to loading module '%s'\n",
+ pmod->name, mod->name);
+ obj->mod = mod;
+ ret = klp_enable_object(pmod, obj);
+ if (ret)
+ pr_warn("failed to apply patch '%s' to module '%s' (%d)\n",
+ pmod->name, mod->name, ret);
+}
+
+static void klp_module_notify_going(struct module *pmod,
+ struct klp_object *obj)
+{
+ struct module *mod = obj->mod;
+ int ret;
+
+ pr_notice("reverting patch '%s' on unloading module '%s'\n",
+ pmod->name, mod->name);
+ ret = klp_disable_object(obj);
+ if (ret)
+ pr_warn("failed to revert patch '%s' on module '%s' (%d)\n",
+ pmod->name, mod->name, ret);
+ obj->mod = NULL;
+}
+
+static int klp_module_notify(struct notifier_block *nb, unsigned long action,
+ void *data)
+{
+ struct module *mod = data;
+ struct klp_patch *patch;
+ struct klp_object *obj;
+
+ if (action != MODULE_STATE_COMING && action != MODULE_STATE_GOING)
+ return 0;
+
+ mutex_lock(&klp_mutex);
+ list_for_each_entry(patch, &klp_patches, list) {
+ if (patch->state == KLP_DISABLED)
+ continue;
+ for (obj = patch->objs; obj->funcs; obj++) {
+ if (!obj->name || strcmp(obj->name, mod->name))
+ continue;
+ if (action == MODULE_STATE_COMING) {
+ obj->mod = mod;
+ klp_module_notify_coming(patch->mod, obj);
+ } else /* MODULE_STATE_GOING */
+ klp_module_notify_going(patch->mod, obj);
+ break;
+ }
+ }
+ mutex_unlock(&klp_mutex);
+ return 0;
+}
+
+static struct notifier_block klp_module_nb = {
+ .notifier_call = klp_module_notify,
+ .priority = INT_MIN+1, /* called late but before ftrace notifier */
+};
+
+/*
+ * Sysfs Interface
+ *
+ * /sys/kernel/livepatch
+ * /sys/kernel/livepatch/<patch>
+ * /sys/kernel/livepatch/<patch>/enabled
+ * /sys/kernel/livepatch/<patch>/<object>
+ * /sys/kernel/livepatch/<patch>/<object>/<func>
+ */
+
+static ssize_t enabled_store(struct kobject *kobj, struct kobj_attribute *attr,
+ const char *buf, size_t count)
+{
+ struct klp_patch *patch;
+ int ret;
+ unsigned long val;
+
+ ret = kstrtoul(buf, 10, &val);
+ if (ret)
+ return -EINVAL;
+
+ if (val != KLP_DISABLED && val != KLP_ENABLED)
+ return -EINVAL;
+
+ patch = container_of(kobj, struct klp_patch, kobj);
+
+ mutex_lock(&klp_mutex);
+ if (val == patch->state) {
+ /* already in requested state */
+ ret = -EINVAL;
+ goto err;
+ }
+
+ if (val == KLP_ENABLED) {
+ ret = __klp_enable_patch(patch);
+ if (ret)
+ goto err;
+ } else {
+ ret = __klp_disable_patch(patch);
+ if (ret)
+ goto err;
+ }
+ mutex_unlock(&klp_mutex);
+ return count;
+err:
+ mutex_unlock(&klp_mutex);
+ return ret;
+}
+
+static ssize_t enabled_show(struct kobject *kobj,
+ struct kobj_attribute *attr, char *buf)
+{
+ struct klp_patch *patch;
+
+ patch = container_of(kobj, struct klp_patch, kobj);
+ return snprintf(buf, PAGE_SIZE-1, "%d\n", patch->state);
+}
+
+static struct kobj_attribute enabled_kobj_attr = __ATTR_RW(enabled);
+static struct attribute *klp_patch_attrs[] = {
+ &enabled_kobj_attr.attr,
+ NULL
+};
+
+
+static void klp_kobj_release_patch(struct kobject *kobj)
+{
+ struct klp_patch *patch;
+
+ patch = container_of(kobj, struct klp_patch, kobj);
+ if (!list_empty(&patch->list))
+ list_del(&patch->list);
+}
+
+static struct kobj_type klp_ktype_patch = {
+ .release = klp_kobj_release_patch,
+ .sysfs_ops = &kobj_sysfs_ops,
+ .default_attrs = klp_patch_attrs
+};
+
+static void klp_kobj_release_func(struct kobject *kobj)
+{
+ struct klp_func *func;
+
+ func = container_of(kobj, struct klp_func, kobj);
+ kfree(func->fops);
+}
+
+static struct kobj_type klp_ktype_func = {
+ .release = klp_kobj_release_func,
+ .sysfs_ops = &kobj_sysfs_ops,
+};
+
+/*
+ * Free all functions' kobjects in the array up to some limit. When limit is
+ * NULL, all kobjects are freed.
+ */
+static void klp_free_funcs_limited(struct klp_object *obj,
+ struct klp_func *limit)
+{
+ struct klp_func *func;
+
+ for (func = obj->funcs; func->old_name && func != limit; func++)
+ kobject_put(&func->kobj);
+}
+
+/*
+ * Free all objects' kobjects in the array up to some limit. When limit is
+ * NULL, all kobjects are freed.
+ */
+static void klp_free_objects_limited(struct klp_patch *patch,
+ struct klp_object *limit)
+{
+ struct klp_object *obj;
+
+ for (obj = patch->objs; obj->funcs && obj != limit; obj++) {
+ klp_free_funcs_limited(obj, NULL);
+ kobject_put(obj->kobj);
+ }
+}
+
+static void klp_free_patch(struct klp_patch *patch)
+{
+ klp_free_objects_limited(patch, NULL);
+ kobject_put(&patch->kobj);
+}
+
+static int klp_init_funcs(struct klp_object *obj)
+{
+ struct klp_func *func;
+ struct ftrace_ops *ops;
+ int ret;
+
+ if (!obj->funcs)
+ return -EINVAL;
+
+ for (func = obj->funcs; func->old_name; func++) {
+ func->state = KLP_DISABLED;
+
+ ops = kzalloc(sizeof(*ops), GFP_KERNEL);
+ if (!ops) {
+ ret = -ENOMEM;
+ goto free;
+ }
+ ops->private = func;
+ ops->func = klp_ftrace_handler;
+ ops->flags = FTRACE_OPS_FL_SAVE_REGS | FTRACE_OPS_FL_DYNAMIC;
+
+ /* sysfs */
+ ret = kobject_init_and_add(&func->kobj, &klp_ktype_func,
+ obj->kobj, func->old_name);
+ if (ret) {
+ kfree(ops);
+ goto free;
+ }
+
+ func->fops = ops;
+ }
+
+ return 0;
+free:
+ klp_free_funcs_limited(obj, func);
+ return ret;
+}
+
+static int klp_init_objects(struct klp_patch *patch)
+{
+ struct klp_object *obj;
+ int ret;
+
+ if (!patch->objs)
+ return -EINVAL;
+
+ for (obj = patch->objs; obj->funcs; obj++) {
+ /* obj->mod set by klp_object_module_get() */
+ obj->state = KLP_DISABLED;
+
+ /* sysfs */
+ obj->kobj = kobject_create_and_add(obj->name, &patch->kobj);
+ if (!obj->kobj)
+ goto free;
+
+ /* init functions */
+ ret = klp_init_funcs(obj);
+ if (ret) {
+ kobject_put(obj->kobj);
+ goto free;
+ }
+ }
+
+ return 0;
+free:
+ klp_free_objects_limited(patch, obj);
+ return -ENOMEM;
+}
+
+static int klp_init_patch(struct klp_patch *patch)
+{
+ int ret;
+
+ if (!patch)
+ return -EINVAL;
+
+ /* init */
+ patch->state = KLP_DISABLED;
+
+ /* sysfs */
+ ret = kobject_init_and_add(&patch->kobj, &klp_ktype_patch,
+ klp_root_kobj, patch->mod->name);
+ if (ret)
+ return ret;
+
+ ret = klp_init_objects(patch);
+ if (ret) {
+ kobject_put(&patch->kobj);
+ return ret;
+ }
+
+ /* add to global list of patches */
+ mutex_lock(&klp_mutex);
+ list_add(&patch->list, &klp_patches);
+ mutex_unlock(&klp_mutex);
+
+ return 0;
+}
+
+/**
+ * klp_register_patch() - registers a patch
+ * @patch: Patch to be registered
+ *
+ * Initializes the data structure associated with the patch and
+ * creates the sysfs interface.
+ *
+ * Return: 0 on success, otherwise error
+ */
+int klp_register_patch(struct klp_patch *patch)
+{
+ int ret;
+
+ if (!patch || !patch->mod || !patch->objs)
+ return -EINVAL;
+
+ /*
+ * A reference is taken on the patch module to prevent it from being
+ * unloaded. Right now, we don't allow patch modules to unload since
+ * there is currently no method to determine if a thread is still
+ * running in the patched code contained in the patch module once
+ * the ftrace registration is successful.
+ */
+ if (!try_module_get(patch->mod))
+ return -ENODEV;
+
+ ret = klp_init_patch(patch);
+ if (ret)
+ module_put(patch->mod);
+
+ return ret;
+}
+EXPORT_SYMBOL_GPL(klp_register_patch);
+
+/**
+ * klp_unregister_patch() - unregisters a patch
+ * @patch: Disabled patch to be unregistered
+ *
+ * Frees the data structures and removes the sysfs interface.
+ *
+ * Return: 0 on success, otherwise error
+ */
+int klp_unregister_patch(struct klp_patch *patch)
+{
+ int ret = 0;
+
+ mutex_lock(&klp_mutex);
+ if (patch->state == KLP_ENABLED) {
+ ret = -EINVAL;
+ goto out;
+ }
+ klp_free_patch(patch);
+out:
+ mutex_unlock(&klp_mutex);
+ return ret;
+}
+EXPORT_SYMBOL_GPL(klp_unregister_patch);
+
+static int klp_init(void)
+{
+ int ret;
+
+ ret = register_module_notifier(&klp_module_nb);
+ if (ret)
+ return ret;
+
+ klp_root_kobj = kobject_create_and_add("livepatch", kernel_kobj);
+ if (!klp_root_kobj) {
+ ret = -ENOMEM;
+ goto unregister;
+ }
+
+ return 0;
+unregister:
+ unregister_module_notifier(&klp_module_nb);
+ return ret;
+}
+
+module_init(klp_init);
--
1.9.3

2014-11-25 17:25:07

[permalink] [raw]

Subject: [PATCHv4 1/3] kernel: add TAINT_LIVEPATCH

This adds a new taint flag to indicate when the kernel or a kernel
module has been live patched. This will provide a clean indication in
bug reports that live patching was used.

Additionally, if the crash occurs in a live patched function, the live
patch module will appear beside the patched function in the backtrace.

Signed-off-by: Seth Jennings <[email protected]>
---
Documentation/oops-tracing.txt | 2 ++
Documentation/sysctl/kernel.txt | 1 +
include/linux/kernel.h | 1 +
kernel/panic.c | 2 ++
4 files changed, 6 insertions(+)

diff --git a/Documentation/oops-tracing.txt b/Documentation/oops-tracing.txt
index beefb9f..f3ac05c 100644
--- a/Documentation/oops-tracing.txt
+++ b/Documentation/oops-tracing.txt
@@ -270,6 +270,8 @@ characters, each representing a particular tainted value.

15: 'L' if a soft lockup has previously occurred on the system.

+ 16: 'K' if the kernel has been live patched.
+
The primary reason for the 'Tainted: ' string is to tell kernel
debuggers if this is a clean kernel or if anything unusual has
occurred. Tainting is permanent: even if an offending module is
diff --git a/Documentation/sysctl/kernel.txt b/Documentation/sysctl/kernel.txt
index 75511ef..83ab256 100644
--- a/Documentation/sysctl/kernel.txt
+++ b/Documentation/sysctl/kernel.txt
@@ -843,6 +843,7 @@ can be ORed together:
8192 - An unsigned module has been loaded in a kernel supporting module
signature.
16384 - A soft lockup has previously occurred on the system.
+32768 - The kernel has been live patched.

==============================================================

diff --git a/include/linux/kernel.h b/include/linux/kernel.h
index 5449d2f..d03e3de 100644
--- a/include/linux/kernel.h
+++ b/include/linux/kernel.h
@@ -471,6 +471,7 @@ extern enum system_states {
#define TAINT_OOT_MODULE 12
#define TAINT_UNSIGNED_MODULE 13
#define TAINT_SOFTLOCKUP 14
+#define TAINT_LIVEPATCH 15

extern const char hex_asc[];
#define hex_asc_lo(x) hex_asc[((x) & 0x0f)]
diff --git a/kernel/panic.c b/kernel/panic.c
index 4d8d6f9..8136ad7 100644
--- a/kernel/panic.c
+++ b/kernel/panic.c
@@ -226,6 +226,7 @@ static const struct tnt tnts[] = {
{ TAINT_OOT_MODULE, 'O', ' ' },
{ TAINT_UNSIGNED_MODULE, 'E', ' ' },
{ TAINT_SOFTLOCKUP, 'L', ' ' },
+ { TAINT_LIVEPATCH, 'K', ' ' },
};

/**
@@ -246,6 +247,7 @@ static const struct tnt tnts[] = {
* 'O' - Out-of-tree module has been loaded.
* 'E' - Unsigned module has been loaded.
* 'L' - A soft lockup has previously occurred.
+ * 'K' - Kernel has been live patched.
*
* The string is overwritten by the next call to print_tainted().
*/
--
1.9.3

2014-11-25 19:26:18

[permalink] [raw]

Subject: Re: [PATCHv4 0/3] Kernel Live Patching

On Tue, 25 Nov 2014, Seth Jennings wrote:

> Masami's IPMODIFY patch is heading for -next via your tree. Once it arrives,
> I'll rebase and make the change to set IPMODIFY. Do not pull this for -next
> yet. This version (v4) is for review and gathering acks.

Thanks for sending out v4 and incorporating the feedback, I really
appreciate your responsiveness!

Anyway, I don't think targetting 3.19 is realistic, given we're currently
already past 3.18-rc6 ... even if we rush it into -next in the coming
days, it will get close to zero exposure in there before the merge window
opens.

I'd like to do quite some more testing and still finish some pending
portions of code reviews on our side (especially to make sure that this
can be easily extended to support any consistency model in the future).

Once we start collecting Reviewed-by's / Acked-by's on this patchset, I
can establish a tree on git.kernel.org that we can use to collect any
followup patches during 3.20 development cycle and send a pull request to
Linus during 3.20 merge window .. if everybody agrees with this course of
action, obviously.

Thanks,

--
Jiri Kosina
SUSE Labs

2014-11-25 22:11:32

[permalink] [raw]

Subject: Re: [PATCHv4 0/3] Kernel Live Patching

On Tue, Nov 25, 2014 at 08:26:22PM +0100, Jiri Kosina wrote:
> On Tue, 25 Nov 2014, Seth Jennings wrote:
>
> > Masami's IPMODIFY patch is heading for -next via your tree. Once it arrives,
> > I'll rebase and make the change to set IPMODIFY. Do not pull this for -next
> > yet. This version (v4) is for review and gathering acks.
>
> Thanks for sending out v4 and incorporating the feedback, I really
> appreciate your responsiveness!
>
> Anyway, I don't think targetting 3.19 is realistic, given we're currently
> already past 3.18-rc6 ... even if we rush it into -next in the coming
> days, it will get close to zero exposure in there before the merge window
> opens.

Agreed. Sorry if I gave the impression that I was trying to rush this
into 3.19. I just wanted to make sure that Steve was aware of the
dependency.

>
> I'd like to do quite some more testing and still finish some pending
> portions of code reviews on our side (especially to make sure that this
> can be easily extended to support any consistency model in the future).

Without knowing how that consistency code will look, how can we "make
sure" that this code can be easily extended to support it? I don't
think we should hold up this first step based on what we think the
consistency code might look like. The code is not that complex right
now. That was the point :) We can always adapt things.

>
> Once we start collecting Reviewed-by's / Acked-by's on this patchset, I
> can establish a tree on git.kernel.org that we can use to collect any
> followup patches during 3.20 development cycle and send a pull request to
> Linus during 3.20 merge window .. if everybody agrees with this course of
> action, obviously.

I was hoping this first step would go into next via Steve's tree and go
upstream for 3.20 (hopefully) from there. I would be against anything
that tries to expand the feature set before this base functionality gets
upstream. However, if we want to have a tree to gather fixes before
3.20, which I think is what you are suggesting, that works for me. We
would need to agree explicitly that, in this tree, patches would need
both a RH and SUSE ack to be accepted.

Thanks,
Seth

>
> Thanks,
>
> --
> Jiri Kosina
> SUSE Labs

2014-11-25 22:22:01

[permalink] [raw]

Subject: Re: [PATCHv4 0/3] Kernel Live Patching

On Tue, 25 Nov 2014, Seth Jennings wrote:

> > I'd like to do quite some more testing and still finish some pending
> > portions of code reviews on our side (especially to make sure that this
> > can be easily extended to support any consistency model in the future).
>
> Without knowing how that consistency code will look, how can we "make
> sure" that this code can be easily extended to support it?

Alright, I probably wasn't precise enough ... I want to make sure that
whatever path (paths) we chose for future consistency models, we will not
have to redesign everything from scratch :)

But this is really rather about finishing some of the reviews that are
still underway on our side.

> > Once we start collecting Reviewed-by's / Acked-by's on this patchset, I
> > can establish a tree on git.kernel.org that we can use to collect any
> > followup patches during 3.20 development cycle and send a pull request to
> > Linus during 3.20 merge window .. if everybody agrees with this course of
> > action, obviously.
>
> I was hoping this first step would go into next via Steve's tree and go
> upstream for 3.20 (hopefully) from there. I would be against anything
> that tries to expand the feature set before this base functionality gets
> upstream.

Fully agreed. No more "core" features than what your current patchset
implements should be added before the first merge.

> However, if we want to have a tree to gather fixes before 3.20, which I
> think is what you are suggesting, that works for me.

Yup, that's what I am suggesting indeed. I think it makes sense to keep
this separate from Steven's tree -- we are ftrace users, but we aren't
touching anything from its internal implementation really.

> We would need to agree explicitly that, in this tree, patches would need
> both a RH and SUSE ack to be accepted.

Oh, absolutely agreed. Without ack from (at least) everyone in
MAINTAINERS, nothing will be applied.

I made this suggestion purely to make things easier for everybody
(especially wrt. merge pull request), not to complicate things by either
playing political games, nor by having to go through proxy tree (tracing),
where the "topical link" is rather loose.

Thanks,

--
Jiri Kosina
SUSE Labs

2014-11-26 09:01:05

[permalink] [raw]

Subject: Re: [PATCHv4 0/3] Kernel Live Patching

(2014/11/26 2:15), Seth Jennings wrote:
> Note to Steve:
> Masami's IPMODIFY patch is heading for -next via your tree. Once it arrives,
> I'll rebase and make the change to set IPMODIFY. Do not pull this for -next
> yet. This version (v4) is for review and gathering acks.

BTW, as we discussed IPMODIFY is an exclusive flag. So if we allocate ftrace_ops
for each function in each patch, it could be conflict each other.
Maybe we need to have another ops hashtable to find such conflict and new
handler to handle it.

Thank you,

--
Masami HIRAMATSU
Software Platform Research Dept. Linux Technology Research Center
Hitachi, Ltd., Yokohama Research Laboratory
E-mail: [email protected]

2014-11-26 09:05:45

[permalink] [raw]

Subject: Re: [PATCHv4 2/3] kernel: add support for live patching

(2014/11/26 2:15), Seth Jennings wrote:
> This commit introduces code for the live patching core. It implements
> an ftrace-based mechanism and kernel interface for doing live patching
> of kernel and kernel module functions.
>
> It represents the greatest common functionality set between kpatch and
> kgraft and can accept patches built using either method.
>
> This first version does not implement any consistency mechanism that
> ensures that old and new code do not run together. In practice, ~90% of
> CVEs are safe to apply in this way, since they simply add a conditional
> check. However, any function change that can not execute safely with
> the old version of the function can _not_ be safely applied in this
> version.

Thank you for updating the patch :)

[...]

> diff --git a/include/linux/livepatch.h b/include/linux/livepatch.h
> new file mode 100644
> index 0000000..4e01b59
> --- /dev/null
> +++ b/include/linux/livepatch.h
> @@ -0,0 +1,121 @@
> +/*
> + * livepatch.h - Kernel Live Patching Core
> + *
> + * Copyright (C) 2014 Seth Jennings <[email protected]>
> + *
> + * 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 _LINUX_LIVEPATCH_H_
> +#define _LINUX_LIVEPATCH_H_
> +
> +#include <linux/module.h>
> +#include <linux/ftrace.h>
> +
> +#if IS_ENABLED(CONFIG_LIVE_PATCHING)
> +
> +#include <asm/livepatch.h>
> +
> +enum klp_state {
> + KLP_DISABLED,
> + KLP_ENABLED
> +};
> +
> +/**
> + * struct klp_func - function structure for live patching
> + * @old_name: name of the function to be patched
> + * @new_func: pointer to the patched function code
> + * @old_addr: a hint conveying at what address the old function
> + * can be found (optional, vmlinux patches only)
> + */
> +struct klp_func {
> + /* external */
> + const char *old_name;
> + void *new_func;
> + /*
> + * The old_addr field is optional and can be used to resolve
> + * duplicate symbol names in the vmlinux object. If this
> + * information is not present, the symbol is located by name
> + * with kallsyms. If the name is not unique and old_addr is
> + * not provided, the patch application fails as there is no
> + * way to resolve the ambiguity.
> + */
> + unsigned long old_addr;
> +
> + /* internal */
> + struct kobject kobj;
> + struct ftrace_ops *fops;
> + enum klp_state state;
> +};
> +
> +/**
> + * struct klp_reloc - relocation structure for live patching
> + * @dest address where the relocation will be written
> + * @src address of the referenced symbol (optional,
> + * vmlinux patches only)
> + * @type ELF relocation type
> + * @name name of the referenced symbol (for lookup/verification)
> + * @addend offset from the referenced symbol
> + * @external symbol is either exported or within the live patch module itself
> + */
> +struct klp_reloc {
> + unsigned long dest;
> + unsigned long src;
> + unsigned long type;
> + const char *name;
> + int addend;

BTW, where would the "addend" come from? I guess we can use "offset" instead.

> + int external;
> +};
> +
> +/* struct klp_object - kernel object structure for live patching
> + * @name module name (or NULL for vmlinux)
> + * @relocs relocation entries to be applied at load time
> + * @funcs function entries for functions to be patched in the object
> + */
> +struct klp_object {
> + /* external */
> + const char *name;
> + struct klp_reloc *relocs;
> + struct klp_func *funcs;
> +
> + /* internal */
> + struct kobject *kobj;
> + struct module *mod;
> + enum klp_state state;
> +};
> +
> +/**
> + * struct klp_patch - patch structure for live patching
> + * @mod reference to the live patch module
> + * @objs object entries for kernel objects to be patched
> + */
> +struct klp_patch {
> + /* external */
> + struct module *mod;
> + struct klp_object *objs;
> +
> + /* internal */
> + struct list_head list;
> + struct kobject kobj;
> + enum klp_state state;
> +};
> +
> +extern int klp_register_patch(struct klp_patch *);
> +extern int klp_unregister_patch(struct klp_patch *);
> +extern int klp_enable_patch(struct klp_patch *);
> +extern int klp_disable_patch(struct klp_patch *);
> +
> +#endif /* CONFIG_LIVE_PATCHING */
> +
> +#endif /* _LINUX_LIVEPATCH_H_ */
> diff --git a/kernel/Makefile b/kernel/Makefile
> index a59481a..616994f 100644
> --- a/kernel/Makefile
> +++ b/kernel/Makefile
> @@ -26,6 +26,7 @@ obj-y += power/
> obj-y += printk/
> obj-y += irq/
> obj-y += rcu/
> +obj-y += livepatch/
>
> obj-$(CONFIG_CHECKPOINT_RESTORE) += kcmp.o
> obj-$(CONFIG_FREEZER) += freezer.o
> diff --git a/kernel/livepatch/Kconfig b/kernel/livepatch/Kconfig
> new file mode 100644
> index 0000000..96da00f
> --- /dev/null
> +++ b/kernel/livepatch/Kconfig
> @@ -0,0 +1,18 @@
> +config ARCH_HAVE_LIVE_PATCHING
> + boolean
> + help
> + Arch supports kernel live patching
> +
> +config LIVE_PATCHING
> + boolean "Kernel Live Patching"
> + depends on DYNAMIC_FTRACE_WITH_REGS
> + depends on MODULES
> + depends on SYSFS
> + depends on KALLSYMS_ALL
> + depends on ARCH_HAVE_LIVE_PATCHING
> + help
> + Say Y here if you want to support kernel live patching.
> + This option has no runtime impact until a kernel "patch"
> + module uses the interface provided by this option to register
> + a patch, causing calls to patched functions to be redirected
> + to new function code contained in the patch module.
> diff --git a/kernel/livepatch/Makefile b/kernel/livepatch/Makefile

BTW, until establishing the consistency models and engines, this would
better depend on EXPERIMENTAL flag :)

Thank you,

--
Masami HIRAMATSU
Software Platform Research Dept. Linux Technology Research Center
Hitachi, Ltd., Yokohama Research Laboratory
E-mail: [email protected]

2014-11-26 09:18:19

[permalink] [raw]

Subject: Re: [PATCHv4 0/3] Kernel Live Patching

On Wed, 26 Nov 2014, Masami Hiramatsu wrote:

> > Note to Steve:
> > Masami's IPMODIFY patch is heading for -next via your tree. Once it arrives,
> > I'll rebase and make the change to set IPMODIFY. Do not pull this for -next
> > yet. This version (v4) is for review and gathering acks.
>
> BTW, as we discussed IPMODIFY is an exclusive flag. So if we allocate
> ftrace_ops for each function in each patch, it could be conflict each
> other.

Yup, this corresponds to what Petr brought up yesterday. There are cases
where all solutions (kpatch, kgraft, klp) would allocate multiple
ftrace_ops for a single function entry (think of patching one function
multiple times in a row).

So it's not as easy as just setting the flag.

> Maybe we need to have another ops hashtable to find such conflict and
> new handler to handle it.

If I understand your proposal correctly, that would sound like a hackish
workaround, trying to basically trick the IPMODIFY flag semantics you just
implemented :)

What I'd propose instead is to make sure that we always have
just a ftrace_ops per function entry, and only update the pointers there
as necessary. Fortunately we can do the switch atomically, by making use
of ->private.

--
Jiri Kosina
SUSE Labs

2014-11-26 09:26:05

[permalink] [raw]

Subject: Re: [PATCHv4 0/3] Kernel Live Patching

On Wed, 26 Nov 2014, Jiri Kosina wrote:

> What I'd propose instead is to make sure that we always have
> just a ftrace_ops per function entry, and only update the pointers there

"... just a _single_ ..."

--
Jiri Kosina
SUSE Labs

2014-11-26 13:37:55

by Jiri Slaby

[permalink] [raw]

Subject: Re: [PATCHv4 2/3] kernel: add support for live patching

Hello,

it is improving a lot over the time. I have still few comments below.

On 11/25/2014, 06:15 PM, Seth Jennings wrote:
> --- /dev/null
> +++ b/arch/x86/include/asm/livepatch.h
> @@ -0,0 +1,40 @@
...
> +#ifndef _ASM_X86_LIVEPATCH_H
> +#define _ASM_X86_LIVEPATCH_H
> +
> +#include <linux/module.h>
> +
> +#ifdef CONFIG_LIVE_PATCHING
> +#ifndef CC_USING_FENTRY
> +#error Your compiler must support -mfentry for live patching to work
> +#endif
> +extern int klp_write_module_reloc(struct module *mod, unsigned long type,
> + unsigned long loc, unsigned long value);
> +
> +#else
> +static inline int klp_write_module_reloc(struct module *mod, unsigned long type,
> + unsigned long loc, unsigned long value)
> +{
> + return -ENOSYS;
> +}
> +#endif
> +
> +#endif /* _ASM_X86_LIVEPATCH_H */
...
> --- /dev/null
> +++ b/arch/x86/kernel/livepatch.c
> @@ -0,0 +1,74 @@
...
> +#include <linux/module.h>
> +#include <linux/uaccess.h>
> +#include <asm/cacheflush.h>
> +#include <asm/page_types.h>

It would make sense to include asm/livepatch.h here. Whenever somebody
changes the prototype of the function or adds a new functionality, it
would force them to write them right.

> +int klp_write_module_reloc(struct module *mod, unsigned long type,
> + unsigned long loc, unsigned long value)
> +{
> + int ret, readonly, numpages, size = 4;
> + unsigned long val;
> + unsigned long core = (unsigned long)mod->module_core;
> + unsigned long core_ro_size = mod->core_ro_size;
> + unsigned long core_size = mod->core_size;
> +
> + switch (type) {
> + case R_X86_64_NONE:
> + return 0;
> + case R_X86_64_64:
> + val = value;
> + size = 8;
> + break;
> + case R_X86_64_32:
> + val = (u32)value;
> + break;
> + case R_X86_64_32S:
> + val = (s32)value;
> + break;
> + case R_X86_64_PC32:
> + val = (u32)(value - loc);
> + break;
> + default:
> + /* unsupported relocation type */
> + return -EINVAL;
> + }
> +
> + if (loc >= core && loc < core + core_ro_size)
> + readonly = 1;
> + else if (loc >= core + core_ro_size && loc < core + core_size)
> + readonly = 0;
> + else
> + /* loc not in expected range */
> + return -EINVAL;
> +
> + numpages = ((loc & PAGE_MASK) == ((loc + size) & PAGE_MASK)) ? 1 : 2;

This deserves a comment or better assignment. Maybe something like:
numpages = DIV_ROUND_UP((loc & ~PAGE_MASK) + size, PAGE_SIZE);

> + if (readonly)
> + set_memory_rw(loc & PAGE_MASK, numpages);
> +
> + ret = probe_kernel_write((void *)loc, &val, size);
> +
> + if (readonly)
> + set_memory_ro(loc & PAGE_MASK, numpages);
> +
> + return ret;
> +}
> diff --git a/include/linux/livepatch.h b/include/linux/livepatch.h
> new file mode 100644
> index 0000000..4e01b59
> --- /dev/null
> +++ b/include/linux/livepatch.h
> @@ -0,0 +1,121 @@
...
> +/**
> + * struct klp_func - function structure for live patching
> + * @old_name: name of the function to be patched
> + * @new_func: pointer to the patched function code
> + * @old_addr: a hint conveying at what address the old function
> + * can be found (optional, vmlinux patches only)
> + */
> +struct klp_func {
> + /* external */
> + const char *old_name;
> + void *new_func;
> + /*
> + * The old_addr field is optional and can be used to resolve
> + * duplicate symbol names in the vmlinux object. If this
> + * information is not present, the symbol is located by name
> + * with kallsyms. If the name is not unique and old_addr is
> + * not provided, the patch application fails as there is no
> + * way to resolve the ambiguity.
> + */
> + unsigned long old_addr;
> +
> + /* internal */
> + struct kobject kobj;
> + struct ftrace_ops *fops;
> + enum klp_state state;
> +};
> +
> +/**
> + * struct klp_reloc - relocation structure for live patching
> + * @dest address where the relocation will be written
> + * @src address of the referenced symbol (optional,
> + * vmlinux patches only)
> + * @type ELF relocation type
> + * @name name of the referenced symbol (for lookup/verification)
> + * @addend offset from the referenced symbol
> + * @external symbol is either exported or within the live patch module itself

Missing colons there and below too 8-).

> +struct klp_reloc {
> + unsigned long dest;
> + unsigned long src;
> + unsigned long type;
> + const char *name;
> + int addend;
> + int external;
> +};
> +
> +/* struct klp_object - kernel object structure for live patching
> + * @name module name (or NULL for vmlinux)
> + * @relocs relocation entries to be applied at load time
> + * @funcs function entries for functions to be patched in the object
> + */
> +struct klp_object {
> + /* external */
> + const char *name;
> + struct klp_reloc *relocs;
> + struct klp_func *funcs;
> +
> + /* internal */
> + struct kobject *kobj;
> + struct module *mod;
> + enum klp_state state;
> +};
> +
> +/**
> + * struct klp_patch - patch structure for live patching
> + * @mod reference to the live patch module
> + * @objs object entries for kernel objects to be patched

Why do you limit documentation only to "API" members? People reading and
working on the code want to know something about the rest too.

> + */
> +struct klp_patch {
> + /* external */
> + struct module *mod;
> + struct klp_object *objs;
> +
> + /* internal */
> + struct list_head list;
> + struct kobject kobj;
> + enum klp_state state;
> +};
...
> --- /dev/null
> +++ b/kernel/livepatch/core.c
> @@ -0,0 +1,807 @@
...
> +static void klp_module_notify_coming(struct module *pmod,
> + struct klp_object *obj)
> +{
> + struct module *mod = obj->mod;
...
> + obj->mod = mod;

No :).

> + ret = klp_enable_object(pmod, obj);
> + if (ret)
> + pr_warn("failed to apply patch '%s' to module '%s' (%d)\n",
> + pmod->name, mod->name, ret);
> +}
...
> +static void klp_kobj_release_patch(struct kobject *kobj)
> +{
> + struct klp_patch *patch;
> +
> + patch = container_of(kobj, struct klp_patch, kobj);
> + if (!list_empty(&patch->list))
> + list_del(&patch->list);

This is broken.

First, the mutex protecting klp_patches is not held on all paths.

Second, ->release is not necessarily called in-line. I mean, whenever
kobject_put returns, this does not mean, the patch will be removed from
the list. Even if it was the last put. This will have unexpected impact
on the assumptions I and others might have about klp_patches. E.g. all
references of objects and funcs in the patch might be gone at any time I
crawl through the list before this ->release is called.

> +}
> +
> +static struct kobj_type klp_ktype_patch = {
> + .release = klp_kobj_release_patch,
> + .sysfs_ops = &kobj_sysfs_ops,
> + .default_attrs = klp_patch_attrs
> +};
...
> +static void klp_free_patch(struct klp_patch *patch)
> +{
> + klp_free_objects_limited(patch, NULL);
> + kobject_put(&patch->kobj);

This one is called with klp_mutex. Do not assume ->release will be
called in-line. It will and it will not. You have to call this outside
the lock, given you need the lock in ->release.

> +}
> +
> +static int klp_init_funcs(struct klp_object *obj)
> +{
> + struct klp_func *func;
> + struct ftrace_ops *ops;
> + int ret;
> +
> + if (!obj->funcs)
> + return -EINVAL;
> +
> + for (func = obj->funcs; func->old_name; func++) {
> + func->state = KLP_DISABLED;
> +
> + ops = kzalloc(sizeof(*ops), GFP_KERNEL);
> + if (!ops) {
> + ret = -ENOMEM;
> + goto free;
> + }
> + ops->private = func;
> + ops->func = klp_ftrace_handler;
> + ops->flags = FTRACE_OPS_FL_SAVE_REGS | FTRACE_OPS_FL_DYNAMIC;

Maybe I missed some discussion. So why cannot func->ops be static?

> + /* sysfs */
> + ret = kobject_init_and_add(&func->kobj, &klp_ktype_func,
> + obj->kobj, func->old_name);
> + if (ret) {
> + kfree(ops);
> + goto free;
> + }
> +
> + func->fops = ops;
> + }
> +
> + return 0;
> +free:
> + klp_free_funcs_limited(obj, func);
> + return ret;
> +}
> +
> +static int klp_init_objects(struct klp_patch *patch)
> +{
> + struct klp_object *obj;
> + int ret;
> +
> + if (!patch->objs)
> + return -EINVAL;

This was already checked in klp_register_patch. Do you anticipate more
paths than that to come here?

> +
> + for (obj = patch->objs; obj->funcs; obj++) {
> + /* obj->mod set by klp_object_module_get() */

I don't see it anywhere.

> + obj->state = KLP_DISABLED;
> +
> + /* sysfs */
> + obj->kobj = kobject_create_and_add(obj->name, &patch->kobj);
> + if (!obj->kobj)
> + goto free;
> +
> + /* init functions */
> + ret = klp_init_funcs(obj);
> + if (ret) {
> + kobject_put(obj->kobj);
> + goto free;
> + }
> + }
> +
> + return 0;
> +free:
> + klp_free_objects_limited(patch, obj);
> + return -ENOMEM;

klp_init_funcs does not always return ENOMEM. The error should be
propagated for easier debugging.

> +}
> +
> +static int klp_init_patch(struct klp_patch *patch)
> +{
> + int ret;
> +
> + if (!patch)
> + return -EINVAL;

Detto as above.

> + /* init */
> + patch->state = KLP_DISABLED;
> +
> + /* sysfs */
> + ret = kobject_init_and_add(&patch->kobj, &klp_ktype_patch,
> + klp_root_kobj, patch->mod->name);
> + if (ret)
> + return ret;
> +
> + ret = klp_init_objects(patch);
> + if (ret) {
> + kobject_put(&patch->kobj);

Hmm, but who initializes patch->list for klp_ktype_patch->release()?

> + return ret;
> + }
> +
> + /* add to global list of patches */

All three comments here are useless as they point out obvious. Drop
them. And similar ones from the routines above.

> + mutex_lock(&klp_mutex);
> + list_add(&patch->list, &klp_patches);
> + mutex_unlock(&klp_mutex);
> +
> + return 0;
> +}
...
> +/**
> + * klp_unregister_patch() - unregisters a patch
> + * @patch: Disabled patch to be unregistered
> + *
> + * Frees the data structures and removes the sysfs interface.
> + *
> + * Return: 0 on success, otherwise error
> + */
> +int klp_unregister_patch(struct klp_patch *patch)
> +{
> + int ret = 0;
> +
> + mutex_lock(&klp_mutex);
> + if (patch->state == KLP_ENABLED) {
> + ret = -EINVAL;

Wouldn't be EBUSY more appropriate?

> + goto out;
> + }
> + klp_free_patch(patch);
> +out:
> + mutex_unlock(&klp_mutex);
> + return ret;
> +}
> +EXPORT_SYMBOL_GPL(klp_unregister_patch);
> +
> +static int klp_init(void)
> +{
> + int ret;
> +
> + ret = register_module_notifier(&klp_module_nb);
> + if (ret)
> + return ret;
> +
> + klp_root_kobj = kobject_create_and_add("livepatch", kernel_kobj);
> + if (!klp_root_kobj) {
> + ret = -ENOMEM;
> + goto unregister;
> + }
> +
> + return 0;
> +unregister:
> + unregister_module_notifier(&klp_module_nb);
> + return ret;

If this really fails, any KLP API function above will crash. We need to
set some flag here and test it above.

> +}
> +
> +module_init(klp_init);
>

thanks,
--
js
suse labs

2014-11-26 14:19:11

[permalink] [raw]

Subject: Re: [PATCHv4 2/3] kernel: add support for live patching

Hi,

thank you for new version, it looks much better now. I still have some
comments though. See below please.

On Tue, 25 Nov 2014, Seth Jennings wrote:

[...]

> diff --git a/include/linux/livepatch.h b/include/linux/livepatch.h
> new file mode 100644
> index 0000000..4e01b59
> --- /dev/null
> +++ b/include/linux/livepatch.h
> @@ -0,0 +1,121 @@
> +/*
> + * livepatch.h - Kernel Live Patching Core
> + *
> + * Copyright (C) 2014 Seth Jennings <[email protected]>
> + *
> + * 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 _LINUX_LIVEPATCH_H_
> +#define _LINUX_LIVEPATCH_H_
> +
> +#include <linux/module.h>
> +#include <linux/ftrace.h>
> +
> +#if IS_ENABLED(CONFIG_LIVE_PATCHING)
> +
> +#include <asm/livepatch.h>
> +
> +enum klp_state {
> + KLP_DISABLED,
> + KLP_ENABLED
> +};
> +
> +/**
> + * struct klp_func - function structure for live patching
> + * @old_name: name of the function to be patched
> + * @new_func: pointer to the patched function code
> + * @old_addr: a hint conveying at what address the old function
> + * can be found (optional, vmlinux patches only)
> + */
> +struct klp_func {
> + /* external */
> + const char *old_name;
> + void *new_func;
> + /*
> + * The old_addr field is optional and can be used to resolve
> + * duplicate symbol names in the vmlinux object. If this
> + * information is not present, the symbol is located by name
> + * with kallsyms. If the name is not unique and old_addr is
> + * not provided, the patch application fails as there is no
> + * way to resolve the ambiguity.
> + */
> + unsigned long old_addr;

I wonder if we really need old_addr as an external field. I assume that
userspace tool in kpatch use it as a "hint" for kernel part and thus
kallsyms is not needed there (and it solves ambiguity problem as well).
But I am not sure if it is gonna be the same in upstream. When kernel is
randomized (CONFIG_RANDOMIZE_BASE is set to 'y', though default is 'n')
old_addr is not usable (and we throw it away in the code). Without
old_addr being set by the user we could spare some of code (calls to
klp_verify_vmlinux_symbol and such).

So the question is whether future userspace tool in upstream would need it
and would use it. Please note that I do not mean it as a kpatch or kgraft
way to do things, I'm just not sure about old_addr being "public" and want
do discuss the options.

The ambiguity of symbols was discussed in some other thread in lkml in
october (I guess) with no conclusion IIRC...

[...]

> +static void klp_ftrace_handler(unsigned long ip, unsigned long parent_ip,
> + struct ftrace_ops *ops, struct pt_regs *regs)

I think we should label ftrace handler as notrace to prevent possible
recursion loop when one would like to patch the handler :). See note in
the comment for register_ftrace_function in kernel/trace/ftrace.c.

> +{
> + struct klp_func *func = ops->private;
> +
> + regs->ip = (unsigned long)func->new_func;
> +}
> +
> +static int klp_enable_func(struct klp_func *func)
> +{
> + int ret;
> +
> + if (WARN_ON(!func->old_addr))
> + return -EINVAL;
> +
> + if (WARN_ON(func->state != KLP_DISABLED))
> + return -EINVAL;
> +
> + ret = ftrace_set_filter_ip(func->fops, func->old_addr, 0, 0);
> + if (ret) {
> + pr_err("failed to set ftrace filter for function '%s' (%d)\n",
> + func->old_name, ret);
> + return ret;
> + }
> + ret = register_ftrace_function(func->fops);
> + if (ret) {
> + pr_err("failed to register ftrace handler for function '%s' (%d)\n",
> + func->old_name, ret);
> + ftrace_set_filter_ip(func->fops, func->old_addr, 1, 0);
> + } else
> + func->state = KLP_ENABLED;

Missing braces for else branch.

> +
> + return ret;
> +}

[...]

> +/**
> + * klp_disable_patch() - disables a registered patch
> + * @patch: The registered, enabled patch to be disabled
> + *
> + * Unregisters the patched functions from ftrace.
> + *
> + * Return: 0 on success, otherwise error
> + */
> +int klp_disable_patch(struct klp_patch *patch)
> +{
> + int ret;
> +
> + mutex_lock(&klp_mutex);
> + if (patch->state == KLP_ENABLED) {
> + ret = -EINVAL;
> + goto out;
> + }

if (patch->state == KLP_DISABLED) { ?

> + ret = __klp_disable_patch(patch);
> +out:
> + mutex_unlock(&klp_mutex);
> + return ret;
> +}
> +EXPORT_SYMBOL_GPL(klp_disable_patch);
> +

[...]
> +static void klp_module_notify_coming(struct module *pmod,
> + struct klp_object *obj)
> +{
> + struct module *mod = obj->mod;
> + int ret;
> +
> + pr_notice("applying patch '%s' to loading module '%s'\n",
> + pmod->name, mod->name);
> + obj->mod = mod;

Still there :)

> + ret = klp_enable_object(pmod, obj);
> + if (ret)
> + pr_warn("failed to apply patch '%s' to module '%s' (%d)\n",
> + pmod->name, mod->name, ret);
> +}
> +
> +static void klp_module_notify_going(struct module *pmod,
> + struct klp_object *obj)
> +{
> + struct module *mod = obj->mod;
> + int ret;
> +
> + pr_notice("reverting patch '%s' on unloading module '%s'\n",
> + pmod->name, mod->name);
> + ret = klp_disable_object(obj);
> + if (ret)
> + pr_warn("failed to revert patch '%s' on module '%s' (%d)\n",
> + pmod->name, mod->name, ret);
> + obj->mod = NULL;
> +}
> +
> +static int klp_module_notify(struct notifier_block *nb, unsigned long action,
> + void *data)
> +{
> + struct module *mod = data;
> + struct klp_patch *patch;
> + struct klp_object *obj;
> +
> + if (action != MODULE_STATE_COMING && action != MODULE_STATE_GOING)
> + return 0;
> +
> + mutex_lock(&klp_mutex);
> + list_for_each_entry(patch, &klp_patches, list) {
> + if (patch->state == KLP_DISABLED)
> + continue;
> + for (obj = patch->objs; obj->funcs; obj++) {
> + if (!obj->name || strcmp(obj->name, mod->name))
> + continue;
> + if (action == MODULE_STATE_COMING) {
> + obj->mod = mod;
> + klp_module_notify_coming(patch->mod, obj);
> + } else /* MODULE_STATE_GOING */
> + klp_module_notify_going(patch->mod, obj);
> + break;
> + }
> + }
> + mutex_unlock(&klp_mutex);
> + return 0;
> +}

[...]
> +static int klp_init_objects(struct klp_patch *patch)
> +{
> + struct klp_object *obj;
> + int ret;
> +
> + if (!patch->objs)
> + return -EINVAL;
> +
> + for (obj = patch->objs; obj->funcs; obj++) {
> + /* obj->mod set by klp_object_module_get() */

Still there. Moreover obj->mod is also set in module notifier.

> + obj->state = KLP_DISABLED;
> +
> + /* sysfs */
> + obj->kobj = kobject_create_and_add(obj->name, &patch->kobj);

There is the problem with obj->name set to NULL (for vmlinux). This
creates subdirectory called '(null)' which is not nice.

> + if (!obj->kobj)
> + goto free;
> +
> + /* init functions */
> + ret = klp_init_funcs(obj);
> + if (ret) {
> + kobject_put(obj->kobj);
> + goto free;
> + }
> + }
> +
> + return 0;
> +free:
> + klp_free_objects_limited(patch, obj);
> + return -ENOMEM;
> +}

> +int klp_register_patch(struct klp_patch *patch)
> +{
> + int ret;
> +
> + if (!patch || !patch->mod || !patch->objs)
> + return -EINVAL;

Check for patch->objs is also above in klp_init_objects, so it can be
removed from here.

> +
> + /*
> + * A reference is taken on the patch module to prevent it from being
> + * unloaded. Right now, we don't allow patch modules to unload since
> + * there is currently no method to determine if a thread is still
> + * running in the patched code contained in the patch module once
> + * the ftrace registration is successful.
> + */
> + if (!try_module_get(patch->mod))
> + return -ENODEV;
> +
> + ret = klp_init_patch(patch);
> + if (ret)
> + module_put(patch->mod);
> +
> + return ret;
> +}
> +EXPORT_SYMBOL_GPL(klp_register_patch);

Thank you again for good work!

--
Miroslav Benes
SUSE Labs

2014-11-26 15:28:38

[permalink] [raw]

Subject: Re: [PATCHv4 0/3] Kernel Live Patching

On Wed, Nov 26, 2014 at 10:18:24AM +0100, Jiri Kosina wrote:
> On Wed, 26 Nov 2014, Masami Hiramatsu wrote:
>
> > > Note to Steve:
> > > Masami's IPMODIFY patch is heading for -next via your tree. Once it arrives,
> > > I'll rebase and make the change to set IPMODIFY. Do not pull this for -next
> > > yet. This version (v4) is for review and gathering acks.
> >
> > BTW, as we discussed IPMODIFY is an exclusive flag. So if we allocate
> > ftrace_ops for each function in each patch, it could be conflict each
> > other.
>
> Yup, this corresponds to what Petr brought up yesterday. There are cases
> where all solutions (kpatch, kgraft, klp) would allocate multiple
> ftrace_ops for a single function entry (think of patching one function
> multiple times in a row).
>
> So it's not as easy as just setting the flag.
>
> > Maybe we need to have another ops hashtable to find such conflict and
> > new handler to handle it.
>
> If I understand your proposal correctly, that would sound like a hackish
> workaround, trying to basically trick the IPMODIFY flag semantics you just
> implemented :)

I think Masami may be proposing something similar to what we do in
kpatch today. We have a single ftrace_ops and handler which is used for
all functions. The handler accesses a global hash of kpatch_func
structs which is indexed by the original function's IP address.

It actually works out pretty well because it nicely encapsulates the
knowledge about which functions are patched in a single place. And it
makes it easy to track function versions (for incremental patching and
rollback).

> What I'd propose instead is to make sure that we always have
> just a ftrace_ops per function entry, and only update the pointers there
> as necessary. Fortunately we can do the switch atomically, by making use
> of ->private.

But how would you update multiple functions atomically, to enforce
per-thread consistency?

--
Josh

2014-11-26 15:41:22

[permalink] [raw]

Subject: Re: [PATCHv4 2/3] kernel: add support for live patching

Hi Miroslav,

Just addressing one of your comments below. I'll let Seth respond to
the others :-)

On Wed, Nov 26, 2014 at 03:19:17PM +0100, Miroslav Benes wrote:
> > +/**
> > + * struct klp_func - function structure for live patching
> > + * @old_name: name of the function to be patched
> > + * @new_func: pointer to the patched function code
> > + * @old_addr: a hint conveying at what address the old function
> > + * can be found (optional, vmlinux patches only)
> > + */
> > +struct klp_func {
> > + /* external */
> > + const char *old_name;
> > + void *new_func;
> > + /*
> > + * The old_addr field is optional and can be used to resolve
> > + * duplicate symbol names in the vmlinux object. If this
> > + * information is not present, the symbol is located by name
> > + * with kallsyms. If the name is not unique and old_addr is
> > + * not provided, the patch application fails as there is no
> > + * way to resolve the ambiguity.
> > + */
> > + unsigned long old_addr;
>
> I wonder if we really need old_addr as an external field. I assume that
> userspace tool in kpatch use it as a "hint" for kernel part and thus
> kallsyms is not needed there (and it solves ambiguity problem as well).
> But I am not sure if it is gonna be the same in upstream. When kernel is
> randomized (CONFIG_RANDOMIZE_BASE is set to 'y', though default is 'n')
> old_addr is not usable (and we throw it away in the code). Without
> old_addr being set by the user we could spare some of code (calls to
> klp_verify_vmlinux_symbol and such).

Even with CONFIG_RANDOMIZE_BASE, the function offsets will be the same
regardless of the base address. So we could still use old_addr to
determine the offset.

> So the question is whether future userspace tool in upstream would need it
> and would use it. Please note that I do not mean it as a kpatch or kgraft
> way to do things, I'm just not sure about old_addr being "public" and want
> do discuss the options.
>
> The ambiguity of symbols was discussed in some other thread in lkml in
> october (I guess) with no conclusion IIRC...

We need to resolve ambiguity somehow, and old_addr is a way to do that.
Do you have any other ideas?

--
Josh

2014-11-26 15:55:44

[permalink] [raw]

Subject: Re: [PATCHv4 0/3] Kernel Live Patching

On Wed, Nov 26, 2014 at 06:00:54PM +0900, Masami Hiramatsu wrote:
> (2014/11/26 2:15), Seth Jennings wrote:
> > Note to Steve:
> > Masami's IPMODIFY patch is heading for -next via your tree. Once it arrives,
> > I'll rebase and make the change to set IPMODIFY. Do not pull this for -next
> > yet. This version (v4) is for review and gathering acks.
>
> BTW, as we discussed IPMODIFY is an exclusive flag. So if we allocate ftrace_ops
> for each function in each patch, it could be conflict each other.
> Maybe we need to have another ops hashtable to find such conflict and new
> handler to handle it.

I agree, but this is not a problem with the current patch set because it
doesn't yet support patching the same function multiple times :-)

--
Josh

2014-11-27 06:12:59

[permalink] [raw]

Subject: Re: [PATCHv4 0/3] Kernel Live Patching

(2014/11/26 18:18), Jiri Kosina wrote:
> On Wed, 26 Nov 2014, Masami Hiramatsu wrote:
>
>>> Note to Steve:
>>> Masami's IPMODIFY patch is heading for -next via your tree. Once it arrives,
>>> I'll rebase and make the change to set IPMODIFY. Do not pull this for -next
>>> yet. This version (v4) is for review and gathering acks.
>>
>> BTW, as we discussed IPMODIFY is an exclusive flag. So if we allocate
>> ftrace_ops for each function in each patch, it could be conflict each
>> other.
>
> Yup, this corresponds to what Petr brought up yesterday. There are cases
> where all solutions (kpatch, kgraft, klp) would allocate multiple
> ftrace_ops for a single function entry (think of patching one function
> multiple times in a row).
>
> So it's not as easy as just setting the flag.
>
>> Maybe we need to have another ops hashtable to find such conflict and
>> new handler to handle it.
>
> If I understand your proposal correctly, that would sound like a hackish
> workaround, trying to basically trick the IPMODIFY flag semantics you just
> implemented :)
>
> What I'd propose instead is to make sure that we always have
> just a ftrace_ops per function entry, and only update the pointers there
> as necessary. Fortunately we can do the switch atomically, by making use
> of ->private.

Would you mean per existing function entry, not per klp-func entry?
If so, it sounds good to me too :)

Thank you,

--
Masami HIRAMATSU
Software Platform Research Dept. Linux Technology Research Center
Hitachi, Ltd., Yokohama Research Laboratory
E-mail: [email protected]

2014-11-27 10:07:10

[permalink] [raw]

Subject: Re: [PATCHv4 0/3] Kernel Live Patching

(2014/11/27 0:27), Josh Poimboeuf wrote:
> On Wed, Nov 26, 2014 at 10:18:24AM +0100, Jiri Kosina wrote:
>> On Wed, 26 Nov 2014, Masami Hiramatsu wrote:
>>
>>>> Note to Steve:
>>>> Masami's IPMODIFY patch is heading for -next via your tree. Once it arrives,
>>>> I'll rebase and make the change to set IPMODIFY. Do not pull this for -next
>>>> yet. This version (v4) is for review and gathering acks.
>>>
>>> BTW, as we discussed IPMODIFY is an exclusive flag. So if we allocate
>>> ftrace_ops for each function in each patch, it could be conflict each
>>> other.
>>
>> Yup, this corresponds to what Petr brought up yesterday. There are cases
>> where all solutions (kpatch, kgraft, klp) would allocate multiple
>> ftrace_ops for a single function entry (think of patching one function
>> multiple times in a row).
>>
>> So it's not as easy as just setting the flag.
>>
>>> Maybe we need to have another ops hashtable to find such conflict and
>>> new handler to handle it.
>>
>> If I understand your proposal correctly, that would sound like a hackish
>> workaround, trying to basically trick the IPMODIFY flag semantics you just
>> implemented :)
>
> I think Masami may be proposing something similar to what we do in
> kpatch today. We have a single ftrace_ops and handler which is used for
> all functions. The handler accesses a global hash of kpatch_func
> structs which is indexed by the original function's IP address.

Hmm, I think both is OK. kpatch method is less memory consuming and
will have a bigger overhead. However, as Steven talked at Plumbers Conf.,
he will introduce a direct code modifying interface for ftrace. After
that is introduced, we don't need to care about performance degradation
by patching :)

> It actually works out pretty well because it nicely encapsulates the
> knowledge about which functions are patched in a single place. And it
> makes it easy to track function versions (for incremental patching and
> rollback).
>
>> What I'd propose instead is to make sure that we always have
>> just a ftrace_ops per function entry, and only update the pointers there
>> as necessary. Fortunately we can do the switch atomically, by making use
>> of ->private.
>
> But how would you update multiple functions atomically, to enforce
> per-thread consistency?

At this point, both can do it atomically. We just need an atomic flag
for applying patches.

Thank you,

--
Masami HIRAMATSU
Software Platform Research Dept. Linux Technology Research Center
Hitachi, Ltd., Yokohama Research Laboratory
E-mail: [email protected]

2014-11-27 10:52:37

[permalink] [raw]

Subject: Re: [PATCHv4 0/3] Kernel Live Patching

On Thu 2014-11-27 19:06:37, Masami Hiramatsu wrote:
> (2014/11/27 0:27), Josh Poimboeuf wrote:
> > On Wed, Nov 26, 2014 at 10:18:24AM +0100, Jiri Kosina wrote:
> >> On Wed, 26 Nov 2014, Masami Hiramatsu wrote:
> >>
> >>>> Note to Steve:
> >>>> Masami's IPMODIFY patch is heading for -next via your tree. Once it arrives,
> >>>> I'll rebase and make the change to set IPMODIFY. Do not pull this for -next
> >>>> yet. This version (v4) is for review and gathering acks.
> >>>
> >>> BTW, as we discussed IPMODIFY is an exclusive flag. So if we allocate
> >>> ftrace_ops for each function in each patch, it could be conflict each
> >>> other.
> >>
> >> Yup, this corresponds to what Petr brought up yesterday. There are cases
> >> where all solutions (kpatch, kgraft, klp) would allocate multiple
> >> ftrace_ops for a single function entry (think of patching one function
> >> multiple times in a row).
> >>
> >> So it's not as easy as just setting the flag.
> >>
> >>> Maybe we need to have another ops hashtable to find such conflict and
> >>> new handler to handle it.
> >>
> >> If I understand your proposal correctly, that would sound like a hackish
> >> workaround, trying to basically trick the IPMODIFY flag semantics you just
> >> implemented :)
> >
> > I think Masami may be proposing something similar to what we do in
> > kpatch today. We have a single ftrace_ops and handler which is used for
> > all functions. The handler accesses a global hash of kpatch_func
> > structs which is indexed by the original function's IP address.
>
> Hmm, I think both is OK. kpatch method is less memory consuming and
> will have a bigger overhead. However, as Steven talked at Plumbers Conf.,
> he will introduce a direct code modifying interface for ftrace. After
> that is introduced, we don't need to care about performance degradation
> by patching :)

Yup, I would prefer to have ftrace_ops per (original) function entry. I mean
that new patches will reuse the existing ftrace_ops for already
patched functions. They will just create new ftrace_ops from the
not-yet-patched symbols.

Using a single ftrace_ops everywhere would kill the win from
Steven's direct ftrace optimization.

> > It actually works out pretty well because it nicely encapsulates the
> > knowledge about which functions are patched in a single place. And it
> > makes it easy to track function versions (for incremental patching and
> > rollback).
> >
> >> What I'd propose instead is to make sure that we always have
> >> just a ftrace_ops per function entry, and only update the pointers there
> >> as necessary. Fortunately we can do the switch atomically, by making use
> >> of ->private.
> >
> > But how would you update multiple functions atomically, to enforce
> > per-thread consistency?
>
> At this point, both can do it atomically. We just need an atomic flag
> for applying patches.

By other words, we would need something like the "kgr_immutable" flag from
kGraft. It will make sure that everybody stays with the current code until
all function entries are updated.

Best Regards,
Petr

2014-11-27 17:05:18

[permalink] [raw]

Subject: Re: [PATCHv4 3/3] samples: add sample live patching module

On Tue 2014-11-25 11:15:09, Seth Jennings wrote:
> Add a sample live patching module.
>
> Signed-off-by: Seth Jennings <[email protected]>
> ---
> MAINTAINERS | 1 +
> samples/Kconfig | 7 +++
> samples/livepatch/Makefile | 1 +
> samples/livepatch/livepatch-sample.c | 87 ++++++++++++++++++++++++++++++++++++
> 4 files changed, 96 insertions(+)
> create mode 100644 samples/livepatch/Makefile
> create mode 100644 samples/livepatch/livepatch-sample.c

[...]

> diff --git a/samples/livepatch/Makefile b/samples/livepatch/Makefile
> new file mode 100644
> index 0000000..7f1cdc1
> --- /dev/null
> +++ b/samples/livepatch/Makefile
> @@ -0,0 +1 @@
> +obj-$(CONFIG_SAMPLE_LIVE_PATCHING) += livepatch-sample.o
> diff --git a/samples/livepatch/livepatch-sample.c b/samples/livepatch/livepatch-sample.c
> new file mode 100644
> index 0000000..21f159d

The Makefile is ignored because there is missing:

diff --git a/samples/Makefile b/samples/Makefile
index 1a60c62e2045..f00257bcc5a7 100644
--- a/samples/Makefile
+++ b/samples/Makefile
@@ -1,4 +1,4 @@
# Makefile for Linux samples code

-obj-$(CONFIG_SAMPLES) += kobject/ kprobes/ trace_events/ \
+obj-$(CONFIG_SAMPLES) += kobject/ kprobes/ trace_events/ livepatch/ \
hw_breakpoint/ kfifo/ kdb/ hidraw/ rpmsg/ seccomp/

Best Regards,
Petr

2014-11-28 02:22:04

[permalink] [raw]

Subject: Re: Re: [PATCHv4 0/3] Kernel Live Patching

(2014/11/27 19:52), Petr Mladek wrote:
> On Thu 2014-11-27 19:06:37, Masami Hiramatsu wrote:
>> (2014/11/27 0:27), Josh Poimboeuf wrote:
>>> On Wed, Nov 26, 2014 at 10:18:24AM +0100, Jiri Kosina wrote:
>>>> On Wed, 26 Nov 2014, Masami Hiramatsu wrote:
>>>>
>>>>>> Note to Steve:
>>>>>> Masami's IPMODIFY patch is heading for -next via your tree. Once it arrives,
>>>>>> I'll rebase and make the change to set IPMODIFY. Do not pull this for -next
>>>>>> yet. This version (v4) is for review and gathering acks.
>>>>>
>>>>> BTW, as we discussed IPMODIFY is an exclusive flag. So if we allocate
>>>>> ftrace_ops for each function in each patch, it could be conflict each
>>>>> other.
>>>>
>>>> Yup, this corresponds to what Petr brought up yesterday. There are cases
>>>> where all solutions (kpatch, kgraft, klp) would allocate multiple
>>>> ftrace_ops for a single function entry (think of patching one function
>>>> multiple times in a row).
>>>>
>>>> So it's not as easy as just setting the flag.
>>>>
>>>>> Maybe we need to have another ops hashtable to find such conflict and
>>>>> new handler to handle it.
>>>>
>>>> If I understand your proposal correctly, that would sound like a hackish
>>>> workaround, trying to basically trick the IPMODIFY flag semantics you just
>>>> implemented :)
>>>
>>> I think Masami may be proposing something similar to what we do in
>>> kpatch today. We have a single ftrace_ops and handler which is used for
>>> all functions. The handler accesses a global hash of kpatch_func
>>> structs which is indexed by the original function's IP address.
>>
>> Hmm, I think both is OK. kpatch method is less memory consuming and
>> will have a bigger overhead. However, as Steven talked at Plumbers Conf.,
>> he will introduce a direct code modifying interface for ftrace. After
>> that is introduced, we don't need to care about performance degradation
>> by patching :)
>
> Yup, I would prefer to have ftrace_ops per (original) function entry. I mean
> that new patches will reuse the existing ftrace_ops for already
> patched functions. They will just create new ftrace_ops from the
> not-yet-patched symbols.

However, too many ftrace_ops will get bigger overhead if conflicts
happened on any entry, since on such entry ftrace walks through
all registered ftrace_ops and checks its filter. It's a downside.
Perhaps, ftrace needs to have 2 different ftrace_ops lists, one
is for managing, and one is for walk through. And the latter list
drops the ftrace_ops which has trampoline and whose all filtered ip
is exclusively used (iow, such ftrace_ops never be hit in the walk
through).

> Using a single ftrace_ops everywhere would kill the win from
> Steven's direct ftrace optimization.

It depends on the implementation and interface. E.g. an explicit
path-change optimization interface for each address with ftrace_ops,
like as
ftrace_change_path_ip(ftrace_ops *ops, unsigned long old_addr,
unsigned long new_addr);

This checks ftrace_ops is already registered and has given old_addr
in filter, ensures no other ftrace_ops are registered on given address,
and then optimizes the fentry call to jump to the new_addr.

Anyway, at this point it is not a major discussion point, it's a
kind of minor implementation issue for performance and memory consuming.
We can switch it without changing API.

Thank you,

>>> It actually works out pretty well because it nicely encapsulates the
>>> knowledge about which functions are patched in a single place. And it
>>> makes it easy to track function versions (for incremental patching and
>>> rollback).
>>>
>>>> What I'd propose instead is to make sure that we always have
>>>> just a ftrace_ops per function entry, and only update the pointers there
>>>> as necessary. Fortunately we can do the switch atomically, by making use
>>>> of ->private.
>>>
>>> But how would you update multiple functions atomically, to enforce
>>> per-thread consistency?
>>
>> At this point, both can do it atomically. We just need an atomic flag
>> for applying patches.
>
> By other words, we would need something like the "kgr_immutable" flag from
> kGraft. It will make sure that everybody stays with the current code until
> all function entries are updated.
>
> Best Regards,
> Petr

--
Masami HIRAMATSU
Software Platform Research Dept. Linux Technology Research Center
Hitachi, Ltd., Yokohama Research Laboratory
E-mail: [email protected]

2014-11-28 17:07:47

[permalink] [raw]

Subject: Re: [PATCHv4 2/3] kernel: add support for live patching

On Tue 2014-11-25 11:15:08, Seth Jennings wrote:
> This commit introduces code for the live patching core. It implements
> an ftrace-based mechanism and kernel interface for doing live patching
> of kernel and kernel module functions.
>
> It represents the greatest common functionality set between kpatch and
> kgraft and can accept patches built using either method.
>
> This first version does not implement any consistency mechanism that
> ensures that old and new code do not run together. In practice, ~90% of
> CVEs are safe to apply in this way, since they simply add a conditional
> check. However, any function change that can not execute safely with
> the old version of the function can _not_ be safely applied in this
> version.

It is nicely improving. I have few more comments.

[...]

> diff --git a/arch/x86/include/asm/livepatch.h b/arch/x86/include/asm/livepatch.h
> new file mode 100644
> index 0000000..38fa496
> --- /dev/null
> +++ b/arch/x86/include/asm/livepatch.h
> @@ -0,0 +1,40 @@
> +/*
> + * livepatch.h - x86-specific Kernel Live Patching Core
> + *
> + * Copyright (C) 2014 Seth Jennings <[email protected]>
> + *
> + * 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_X86_LIVEPATCH_H
> +#define _ASM_X86_LIVEPATCH_H
> +
> +#include <linux/module.h>
> +
> +#ifdef CONFIG_LIVE_PATCHING
> +#ifndef CC_USING_FENTRY
> +#error Your compiler must support -mfentry for live patching to work
> +#endif
> +extern int klp_write_module_reloc(struct module *mod, unsigned long type,
> + unsigned long loc, unsigned long value);
> +
> +#else
> +static inline int klp_write_module_reloc(struct module *mod, unsigned long type,
> + unsigned long loc, unsigned long value)
> +{
> + return -ENOSYS;
> +}
> +#endif

I would avoid the #else part. IMHO, it is better to report such
problems at compile time. The alternative could be added later
if anyone really wants to use it outside LivePatch and do
some minimal build without this feature.

> +
> +#endif /* _ASM_X86_LIVEPATCH_H */

[...]

> --- /dev/null
> +++ b/arch/x86/kernel/livepatch.c
> @@ -0,0 +1,74 @@
> +/*
> + * livepatch.c - x86-specific Kernel Live Patching Core
> + *
> + * Copyright (C) 2014 Seth Jennings <[email protected]>
> + *
> + * 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/uaccess.h>
> +#include <asm/cacheflush.h>
> +#include <asm/page_types.h>
> +

Please, add some comments that would explain the purpose
of the function and parameters.

> +int klp_write_module_reloc(struct module *mod, unsigned long type,
> + unsigned long loc, unsigned long value)
> +{
> + int ret, readonly, numpages, size = 4;

I would use bool for "readonly" and change the code to use "true" and "false".

> + unsigned long val;
> + unsigned long core = (unsigned long)mod->module_core;
> + unsigned long core_ro_size = mod->core_ro_size;
> + unsigned long core_size = mod->core_size;
> +
> + switch (type) {
> + case R_X86_64_NONE:
> + return 0;
> + case R_X86_64_64:
> + val = value;
> + size = 8;
> + break;
> + case R_X86_64_32:
> + val = (u32)value;
> + break;
> + case R_X86_64_32S:
> + val = (s32)value;
> + break;
> + case R_X86_64_PC32:
> + val = (u32)(value - loc);
> + break;
> + default:
> + /* unsupported relocation type */
> + return -EINVAL;
> + }
> +
> + if (loc >= core && loc < core + core_ro_size)
> + readonly = 1;
> + else if (loc >= core + core_ro_size && loc < core + core_size)
> + readonly = 0;
> + else
> + /* loc not in expected range */
> + return -EINVAL;

Small optimization would be:

if (loc < core || loc >= core + core_size)
/* loc does not point to any symbol inside the module */
return -EINVAL;

if (loc < core + core_ro_size)
readonly = 1;
else
readonly = 0;

> + numpages = ((loc & PAGE_MASK) == ((loc + size) & PAGE_MASK)) ? 1 : 2;
> +
> + if (readonly)
> + set_memory_rw(loc & PAGE_MASK, numpages);
> +
> + ret = probe_kernel_write((void *)loc, &val, size);
> +
> + if (readonly)
> + set_memory_ro(loc & PAGE_MASK, numpages);
> +
> + return ret;
> +}
> diff --git a/include/linux/livepatch.h b/include/linux/livepatch.h
> new file mode 100644
> index 0000000..4e01b59
> --- /dev/null
> +++ b/include/linux/livepatch.h
> @@ -0,0 +1,121 @@
> +/*
> + * livepatch.h - Kernel Live Patching Core
> + *
> + * Copyright (C) 2014 Seth Jennings <[email protected]>
> + *
> + * 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 _LINUX_LIVEPATCH_H_
> +#define _LINUX_LIVEPATCH_H_
> +
> +#include <linux/module.h>
> +#include <linux/ftrace.h>
> +
> +#if IS_ENABLED(CONFIG_LIVE_PATCHING)
> +
> +#include <asm/livepatch.h>
> +

[...]

> +/**
> + * struct klp_reloc - relocation structure for live patching
> + * @dest address where the relocation will be written
> + * @src address of the referenced symbol (optional,
> + * vmlinux patches only)
> + * @type ELF relocation type
> + * @name name of the referenced symbol (for lookup/verification)
> + * @addend offset from the referenced symbol
> + * @external symbol is either exported or within the live patch module itself
> + */
> +struct klp_reloc {
> + unsigned long dest;
> + unsigned long src;

The names are a bit confusing. It makes the feeling that we copy some
data from "src" to "dest". I wonder if we could get some more
descriptive names from the reloc function.

I would use the same naming scheme that is used by
arch_kexec_apply_relocations_add() and apply_relocate_add()
and also by the new klp_write_module_reloc()
I mean:

dest -> loc
src -> val

> + unsigned long type;
> + const char *name;
> + int addend;

After all, I would keep the "addend". It sounds strange but it seems to
be the name used by other similar functions as well. See also
include/uapi/linux/elf.h

> + int external;
> +};
> +
> +/* struct klp_object - kernel object structure for live patching

/**

> + * @name module name (or NULL for vmlinux)
> + * @relocs relocation entries to be applied at load time
> + * @funcs function entries for functions to be patched in the object
> + */
> +struct klp_object {
> + /* external */
> + const char *name;
> + struct klp_reloc *relocs;
> + struct klp_func *funcs;
> +
> + /* internal */
> + struct kobject *kobj;
> + struct module *mod;
> + enum klp_state state;
> +};
> +
> +/**
> + * struct klp_patch - patch structure for live patching
> + * @mod reference to the live patch module
> + * @objs object entries for kernel objects to be patched
> + */
> +struct klp_patch {
> + /* external */
> + struct module *mod;
> + struct klp_object *objs;
> +
> + /* internal */
> + struct list_head list;
> + struct kobject kobj;
> + enum klp_state state;
> +};
> +
> +extern int klp_register_patch(struct klp_patch *);
> +extern int klp_unregister_patch(struct klp_patch *);
> +extern int klp_enable_patch(struct klp_patch *);
> +extern int klp_disable_patch(struct klp_patch *);
> +
> +#endif /* CONFIG_LIVE_PATCHING */
> +
> +#endif /* _LINUX_LIVEPATCH_H_ */

[...]

> --- /dev/null
> +++ b/kernel/livepatch/core.c
> @@ -0,0 +1,807 @@
> +/*
> + * core.c - Kernel Live Patching Core
> + *
> + * Copyright (C) 2014 Seth Jennings <[email protected]>
> + *
> + * 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/>.
> + */
> +
> +#define pr_fmt(fmt) KBUILD_MODNAME ": " fmt
> +
> +#include <linux/module.h>
> +#include <linux/kernel.h>
> +#include <linux/mutex.h>
> +#include <linux/slab.h>
> +#include <linux/ftrace.h>
> +#include <linux/list.h>
> +#include <linux/kallsyms.h>
> +#include <linux/livepatch.h>
> +
> +/*
> + * The klp_mutex protects the klp_patches list and state transitions of any
> + * structure reachable from the patches list. References to any structure must
> + * be obtained under mutex protection.
> + */
> +
> +static DEFINE_MUTEX(klp_mutex);
> +static LIST_HEAD(klp_patches);
> +
> +static struct kobject *klp_root_kobj;
> +
> +/* sets obj->mod if object is not vmlinux and module is found */
> +static bool klp_find_object_module(struct klp_object *obj)
> +{
> + if (!obj->name)
> + return 1;
> +
> + mutex_lock(&module_mutex);
> + /*
> + * We don't need to take a reference on the module here because we have
> + * the klp_mutex, which is also taken by the module notifier. This
> + * prevents any module from unloading until we release the klp_mutex.
> + */
> + obj->mod = find_module(obj->name);
> + mutex_unlock(&module_mutex);
> +
> + return !!obj->mod;

I know that this is effective to return boolean here because
it handles also patch against the kernel core (vmlinux). But
the logic looks tricky. I would prefer a cleaner design and
use this function only to set obj->mod.

I wanted to see how it would look like, so I will send a patch for
this in a separate mail.

> +}

[...]

> +static int klp_write_object_relocations(struct module *pmod,
> + struct klp_object *obj)
> +{
> + int ret;
> + struct klp_reloc *reloc;

We should add some consistency checks here. I will send
a patch for this as well.

> + for (reloc = obj->relocs; reloc->name; reloc++) {

This might deserve a comment. Something like:

/*
* Only symbols from modules need to be relocated.
* The symbols from the core kernel binary (vmlinux) are
* only used to check consistency between the patch and
* the patched kernel.
*/
> + if (!obj->name) {
> + ret = klp_verify_vmlinux_symbol(reloc->name,
> + reloc->src);
> + if (ret)
> + return ret;
> + } else {
> + /* module, reloc->src needs to be discovered */
> + if (reloc->external)
> + ret = klp_find_external_symbol(pmod,
> + reloc->name,
> + &reloc->src);
> + else
> + ret = klp_find_symbol(obj->mod->name,
> + reloc->name,
> + &reloc->src);

I was confused that klp_find_external_symbol() is actually more
generic than klp_find_symbol(). It might help to rename:
klp_find_symbol() -> klp_find_object_symbol()

> + if (ret)
> + return ret;
> + }
> + ret = klp_write_module_reloc(pmod, reloc->type, reloc->dest,
> + reloc->src + reloc->addend);
> + if (ret) {
> + pr_err("relocation failed for symbol '%s' at 0x%016lx (%d)\n",
> + reloc->name, reloc->src, ret);
> + return ret;
> + }
> + }
> +
> + return 0;
> +}
> +

[...]

> +static int klp_disable_object(struct klp_object *obj)
> +{
> + struct klp_func *func;
> + int ret;

if (WARN_ON(obj->state != KLP_ENABLED))
return -EINVAL;

> +
> + for (func = obj->funcs; func->old_name; func++) {
> + if (func->state != KLP_ENABLED)
> + continue;
> + ret = klp_disable_func(func);
> + if (ret)
> + return ret;
> + if (obj->name)
> + func->old_addr = 0;
> + }
> + obj->state = KLP_DISABLED;
> +
> + return 0;
> +}
> +
> +static int klp_enable_object(struct module *pmod, struct klp_object *obj)
> +{
> + struct klp_func *func;
> + int ret;
> +
if (WARN_ON(obj->state != KLP_DISABLED))
return -EINVAL;

> + if (WARN_ON(!obj->mod && obj->name))
> + return -EINVAL;
> +
> + if (obj->relocs) {
> + ret = klp_write_object_relocations(pmod, obj);

I was curious why the relocation is here. I think that the motivation
was to safe the call when handling comming modules.

IMHO, more clean solution would be to do this in klp_init_object().
It will also cause removing the "pmod" parameter and this function
will be more symmetric with klp_disable_object();

I was curious how it would work, so I prepared a patch. I will send
it in separate mail.

> + if (ret)
> + goto unregister;
> + }
> +
> + for (func = obj->funcs; func->old_name; func++) {
> + ret = klp_find_verify_func_addr(func, obj->name);
> + if (ret)
> + goto unregister;
> +
> + ret = klp_enable_func(func);
> + if (ret)
> + goto unregister;
> + }
> + obj->state = KLP_ENABLED;
> +
> + return 0;
> +unregister:
> + WARN_ON(klp_disable_object(obj));
> + return ret;
> +}
> +
> +static int __klp_disable_patch(struct klp_patch *patch)
> +{
> + struct klp_object *obj;
> + int ret;
> +
if (WARN_ON(patch->state != KLP_ENABLED))
return -EINVAL;

> + pr_notice("disabling patch '%s'\n", patch->mod->name);
> +
> + for (obj = patch->objs; obj->funcs; obj++) {
> + if (obj->state != KLP_ENABLED)
> + continue;
> + ret = klp_disable_object(obj);
> + if (ret)
> + return ret;
> + }
> + patch->state = KLP_DISABLED;
> +
> + return 0;
> +}
> +
> +/**
> + * klp_disable_patch() - disables a registered patch
> + * @patch: The registered, enabled patch to be disabled
> + *
> + * Unregisters the patched functions from ftrace.
> + *
> + * Return: 0 on success, otherwise error
> + */
> +int klp_disable_patch(struct klp_patch *patch)
> +{
> + int ret;
> +
> + mutex_lock(&klp_mutex);
> + if (patch->state == KLP_ENABLED) {

IMHO, this check belongs to the __klp_disable_patch().
Same approach that is already used in klp_enable_patch().

> + ret = -EINVAL;
> + goto out;
> + }
> + ret = __klp_disable_patch(patch);
> +out:
> + mutex_unlock(&klp_mutex);
> + return ret;
> +}
> +EXPORT_SYMBOL_GPL(klp_disable_patch);
> +
> +static int __klp_enable_patch(struct klp_patch *patch)
> +{
> + struct klp_object *obj;
> + int ret;
> +
> + if (WARN_ON(patch->state != KLP_DISABLED))
> + return -EINVAL;
> +
> + pr_notice_once("tainting kernel with TAINT_LIVEPATCH\n");
> + add_taint(TAINT_LIVEPATCH, LOCKDEP_STILL_OK);
> +
> + pr_notice("enabling patch '%s'\n", patch->mod->name);
> +
> + for (obj = patch->objs; obj->funcs; obj++) {
> + if (!klp_find_object_module(obj))
> + continue;
> + ret = klp_enable_object(patch->mod, obj);
> + if (ret)
> + goto unregister;
> + }
> + patch->state = KLP_ENABLED;
> + return 0;
> +
> +unregister:
> + WARN_ON(__klp_disable_patch(patch));
> + return ret;
> +}
> +
> +/**
> + * klp_enable_patch() - enables a registered patch
> + * @patch: The registered, disabled patch to be enabled
> + *
> + * Performs the needed symbol lookups and code relocations,
> + * then registers the patched functions with ftrace.
> + *
> + * Return: 0 on success, otherwise error
> + */
> +int klp_enable_patch(struct klp_patch *patch)
> +{
> + int ret;
> +
> + mutex_lock(&klp_mutex);
> + ret = __klp_enable_patch(patch);
> + mutex_unlock(&klp_mutex);
> +
> + return ret;
> +}
> +EXPORT_SYMBOL_GPL(klp_enable_patch);
> +
> +static void klp_module_notify_coming(struct module *pmod,
> + struct klp_object *obj)

I was confused that this function handles only one patch. Then
I found that it is called for each patch from klp_module_notify().

What about renaming to klp_init_and_enable_object_delayed() or
klp_module_notify_init_and_enable_object() or so.

In fact, we might want to split this into two functions handling
the init and enable part. It will make the name shorter and it
will be more in sync with the rest of the code.

> +{
> + struct module *mod = obj->mod;
> + int ret;
> +
> + pr_notice("applying patch '%s' to loading module '%s'\n",
> + pmod->name, mod->name);
> + obj->mod = mod;

It means that we do not need to call klp_find_object_module() in
klp_enable_object(). This will be solved with my patch moving
the relocation to the init phase as well.

If you agree and we do relocation here. It would make sense to somehow
share the code with klp_init_object().

> + ret = klp_enable_object(pmod, obj);
> + if (ret)
> + pr_warn("failed to apply patch '%s' to module '%s' (%d)\n",
> + pmod->name, mod->name, ret);
> +}
> +
> +static void klp_module_notify_going(struct module *pmod,
> + struct klp_object *obj)
> +{

Same problem here. I would use a better same, see the comments
klp_module_notify_coming() above.

> + struct module *mod = obj->mod;
> + int ret;
> +
> + pr_notice("reverting patch '%s' on unloading module '%s'\n",
> + pmod->name, mod->name);
> + ret = klp_disable_object(obj);
> + if (ret)
> + pr_warn("failed to revert patch '%s' on module '%s' (%d)\n",
> + pmod->name, mod->name, ret);
> + obj->mod = NULL;
> +}
> +
> +static int klp_module_notify(struct notifier_block *nb, unsigned long action,
> + void *data)
> +{
> + struct module *mod = data;
> + struct klp_patch *patch;
> + struct klp_object *obj;
> +
> + if (action != MODULE_STATE_COMING && action != MODULE_STATE_GOING)
> + return 0;
> +
> + mutex_lock(&klp_mutex);
> + list_for_each_entry(patch, &klp_patches, list) {
> + if (patch->state == KLP_DISABLED)
> + continue;
> + for (obj = patch->objs; obj->funcs; obj++) {
> + if (!obj->name || strcmp(obj->name, mod->name))
> + continue;
> + if (action == MODULE_STATE_COMING) {
> + obj->mod = mod;
> + klp_module_notify_coming(patch->mod, obj);
> + } else /* MODULE_STATE_GOING */
> + klp_module_notify_going(patch->mod, obj);
> + break;
> + }
> + }
> + mutex_unlock(&klp_mutex);
> + return 0;
> +}
> +
[...]

> +static int klp_init_patch(struct klp_patch *patch)
> +{
> + int ret;
> +
> + if (!patch)
> + return -EINVAL;

I would feel more comfortable if the take the lock already here.
At least, it might be needed to avoid race with klp_module_notify()
stuff. It will block any module load/remove until the klp structures
are consistent.

> + /* init */
> + patch->state = KLP_DISABLED;
> +
> + /* sysfs */
> + ret = kobject_init_and_add(&patch->kobj, &klp_ktype_patch,
> + klp_root_kobj, patch->mod->name);
> + if (ret)
> + return ret;
> +
> + ret = klp_init_objects(patch);
> + if (ret) {
> + kobject_put(&patch->kobj);
> + return ret;
> + }
> +
> + /* add to global list of patches */
> + mutex_lock(&klp_mutex);
> + list_add(&patch->list, &klp_patches);
> + mutex_unlock(&klp_mutex);
> +
> + return 0;
> +}

Anyway, thanks for working on it. The progress is great.

BTW: I wonder what is the preferred way of cooperation. I prepared
patch for changes in the program logic. Here it might be worth
solving the potential conflicts. But I made only comments about
the renaming and other simple changes. I think that these are easier
do directly without having to solve conflict.

Best Regards,
Petr

2014-11-28 17:14:41

[permalink] [raw]

Subject: [PATCH] livepatch: clean up klp_find_object_module() usage: was: Re: [PATCHv4 2/3] kernel: add support for live patching

On Fri 2014-11-28 18:07:37, Petr Mladek wrote:
> On Tue 2014-11-25 11:15:08, Seth Jennings wrote:
> > This commit introduces code for the live patching core. It implements
> > an ftrace-based mechanism and kernel interface for doing live patching
> > of kernel and kernel module functions.

[...]

> > +/* sets obj->mod if object is not vmlinux and module is found */
> > +static bool klp_find_object_module(struct klp_object *obj)
> > +{
> > + if (!obj->name)
> > + return 1;
> > +
> > + mutex_lock(&module_mutex);
> > + /*
> > + * We don't need to take a reference on the module here because we have
> > + * the klp_mutex, which is also taken by the module notifier. This
> > + * prevents any module from unloading until we release the klp_mutex.
> > + */
> > + obj->mod = find_module(obj->name);
> > + mutex_unlock(&module_mutex);
> > +
> > + return !!obj->mod;
>
> I know that this is effective to return boolean here because
> it handles also patch against the kernel core (vmlinux). But
> the logic looks tricky. I would prefer a cleaner design and
> use this function only to set obj->mod.
>
> I wanted to see how it would look like, so I will send a patch for
> this in a separate mail.

The patch is below. Of course, merge it into the original
patch if you agree with the idea, please.

>From 93eb9f9a25ad8aa0301e246f7685d3e787037566 Mon Sep 17 00:00:00 2001
From: Petr Mladek <[email protected]>
Date: Fri, 28 Nov 2014 15:32:27 +0100
Subject: [PATCH 1/2] livepatch: clean up klp_find_object_module() usage

The function klp_find_object_module() looks quite tricky. It has two effects:
sets obj->mod and decides whether the module is available or not. The second
effect is the tricky part because it handles also the code kernel code "vmlinux"
and is not module related. It causes returning bool, and doing the crazy double
negation.

This patch tries to make a bit cleaner design:

1. klp_find_object_module() handles only obj->mod. It returns
the pointer or NULL.

2. It modifies klp_enable_object() to do nothing when the related
module has not been loaded yet.

3. The result is that the return value klp_find_object_module() is
not used in the end.

We lose a check for potential klp_enable_object() misuse but it makes the code
easier. In fact, the check for unloaded module is rather long. We might want
to make it easier using some extra flag or another state of the object.
Such flag might be used for the check of misuse.

Signed-off-by: Petr Mladek <[email protected]>

---
kernel/livepatch/core.c | 14 +++++++-------
1 file changed, 7 insertions(+), 7 deletions(-)

diff --git a/kernel/livepatch/core.c b/kernel/livepatch/core.c
index 8e2e8cd242f5..9b1601729014 100644
--- a/kernel/livepatch/core.c
+++ b/kernel/livepatch/core.c
@@ -40,10 +40,10 @@ static LIST_HEAD(klp_patches);
static struct kobject *klp_root_kobj;

/* sets obj->mod if object is not vmlinux and module is found */
-static bool klp_find_object_module(struct klp_object *obj)
+static struct module *klp_find_object_module(struct klp_object *obj)
{
if (!obj->name)
- return 1;
+ return NULL;

mutex_lock(&module_mutex);
/*
@@ -54,7 +54,7 @@ static bool klp_find_object_module(struct klp_object *obj)
obj->mod = find_module(obj->name);
mutex_unlock(&module_mutex);

- return !!obj->mod;
+ return obj->mod;
}

struct klp_find_arg {
@@ -318,8 +318,9 @@ static int klp_enable_object(struct module *pmod, struct klp_object *obj)
struct klp_func *func;
int ret;

- if (WARN_ON(!obj->mod && obj->name))
- return -EINVAL;
+ /* nope when the patched module has not been loaded yet */
+ if (obj->name && !obj->mod)
+ return 0;

if (obj->relocs) {
ret = klp_write_object_relocations(pmod, obj);
@@ -401,8 +402,7 @@ static int __klp_enable_patch(struct klp_patch *patch)
pr_notice("enabling patch '%s'\n", patch->mod->name);

for (obj = patch->objs; obj->funcs; obj++) {
- if (!klp_find_object_module(obj))
- continue;
+ klp_find_object_module(obj);
ret = klp_enable_object(patch->mod, obj);
if (ret)
goto unregister;
--
1.8.5.2

2014-11-28 17:19:12

[permalink] [raw]

Subject: [PATCH] livepatch: do relocation when initializing the patch: was: Re: [PATCHv4 2/3] kernel: add support for live patching

On Fri 2014-11-28 18:07:37, Petr Mladek wrote:
> On Tue 2014-11-25 11:15:08, Seth Jennings wrote:
> > This commit introduces code for the live patching core. It implements
> > an ftrace-based mechanism and kernel interface for doing live patching
> > of kernel and kernel module functions.

[...]

> > +static int klp_enable_object(struct module *pmod, struct klp_object *obj)
> > +{
> > + struct klp_func *func;
> > + int ret;
> > +
> if (WARN_ON(obj->state != KLP_DISABLED))
> return -EINVAL;
>
> > + if (WARN_ON(!obj->mod && obj->name))
> > + return -EINVAL;
> > +
> > + if (obj->relocs) {
> > + ret = klp_write_object_relocations(pmod, obj);
>
> I was curious why the relocation is here. I think that the motivation
> was to safe the call when handling comming modules.
>
> IMHO, more clean solution would be to do this in klp_init_object().
> It will also cause removing the "pmod" parameter and this function
> will be more symmetric with klp_disable_object();
>
> I was curious how it would work, so I prepared a patch. I will send
> it in separate mail.

The patch is below. Again, merge it into the original patch if you
agree with the idea, please.

>From ba3b08938b6f6f1a041af645ff43825f2ec1222f Mon Sep 17 00:00:00 2001
From: Petr Mladek <[email protected]>
Date: Fri, 28 Nov 2014 15:57:23 +0100
Subject: [PATCH 2/2] livepatch: do relocation when initializing the patch

I think that the relocation is done in klp_enable_object() because
it safes the duplication in klp_module_notify_coming().

But I think that the relocation logically belongs to the init phase.
It will remove duplicate relocation if we allow repeated enable/disable
calls for the same patch. Also it removes the extra "pmod" parameter
from klp_enable_object() and makes it more symmetric with klp_disable_object().

This patch moves also the klp_find_object_module() to the init phase
where it belongs.

Finally, it moves some checks from callers to klp_write_object_relocation().
They must be there in each case. It makes the code easier when calling
from more locations.

Signed-off-by: Petr Mladek <[email protected]>

---
kernel/livepatch/core.c | 41 ++++++++++++++++++++++++++---------------
1 file changed, 26 insertions(+), 15 deletions(-)

diff --git a/kernel/livepatch/core.c b/kernel/livepatch/core.c
index 9b1601729014..688a6b66e72f 100644
--- a/kernel/livepatch/core.c
+++ b/kernel/livepatch/core.c
@@ -204,6 +204,14 @@ static int klp_write_object_relocations(struct module *pmod,
int ret;
struct klp_reloc *reloc;

+ /* nope when the patched module has not been loaded yet */
+ if (obj->name && !obj->mod)
+ return 0;
+
+ /* be happy when there is nothing to do */
+ if (!obj->relocs)
+ return 0;
+
for (reloc = obj->relocs; reloc->name; reloc++) {
if (!obj->name) {
ret = klp_verify_vmlinux_symbol(reloc->name,
@@ -313,7 +321,7 @@ static int klp_disable_object(struct klp_object *obj)
return 0;
}

-static int klp_enable_object(struct module *pmod, struct klp_object *obj)
+static int klp_enable_object(struct klp_object *obj)
{
struct klp_func *func;
int ret;
@@ -322,12 +330,6 @@ static int klp_enable_object(struct module *pmod, struct klp_object *obj)
if (obj->name && !obj->mod)
return 0;

- if (obj->relocs) {
- ret = klp_write_object_relocations(pmod, obj);
- if (ret)
- goto unregister;
- }
-
for (func = obj->funcs; func->old_name; func++) {
ret = klp_find_verify_func_addr(func, obj->name);
if (ret)
@@ -402,8 +404,7 @@ static int __klp_enable_patch(struct klp_patch *patch)
pr_notice("enabling patch '%s'\n", patch->mod->name);

for (obj = patch->objs; obj->funcs; obj++) {
- klp_find_object_module(obj);
- ret = klp_enable_object(patch->mod, obj);
+ ret = klp_enable_object(obj);
if (ret)
goto unregister;
}
@@ -445,10 +446,17 @@ static void klp_module_notify_coming(struct module *pmod,
pr_notice("applying patch '%s' to loading module '%s'\n",
pmod->name, mod->name);
obj->mod = mod;
- ret = klp_enable_object(pmod, obj);
+ ret = klp_write_object_relocations(pmod, obj);
if (ret)
- pr_warn("failed to apply patch '%s' to module '%s' (%d)\n",
- pmod->name, mod->name, ret);
+ goto err;
+
+ ret = klp_enable_object(obj);
+ if (!ret)
+ return;
+
+err:
+ pr_warn("failed to apply patch '%s' to module '%s' (%d)\n",
+ pmod->name, mod->name, ret);
}

static void klp_module_notify_going(struct module *pmod,
@@ -674,15 +682,18 @@ static int klp_init_objects(struct klp_patch *patch)
return -EINVAL;

for (obj = patch->objs; obj->funcs; obj++) {
- /* obj->mod set by klp_object_module_get() */
obj->state = KLP_DISABLED;
+ klp_find_object_module(obj);

- /* sysfs */
+ ret = klp_write_object_relocations(patch->mod, obj);
+ if (ret)
+ goto free;
+
+ /* sysfs stuff */
obj->kobj = kobject_create_and_add(obj->name, &patch->kobj);
if (!obj->kobj)
goto free;

- /* init functions */
ret = klp_init_funcs(obj);
if (ret) {
kobject_put(obj->kobj);
--
1.8.5.2

2014-12-01 12:08:42

[permalink] [raw]

Subject: Re: [PATCH] livepatch: clean up klp_find_object_module() usage: was: Re: [PATCHv4 2/3] kernel: add support for live patching

On Fri, 28 Nov 2014, Petr Mladek wrote:

> On Fri 2014-11-28 18:07:37, Petr Mladek wrote:
> > On Tue 2014-11-25 11:15:08, Seth Jennings wrote:
> > > This commit introduces code for the live patching core. It implements
> > > an ftrace-based mechanism and kernel interface for doing live patching
> > > of kernel and kernel module functions.
>
> [...]
>
> > > +/* sets obj->mod if object is not vmlinux and module is found */
> > > +static bool klp_find_object_module(struct klp_object *obj)
> > > +{
> > > + if (!obj->name)
> > > + return 1;
> > > +
> > > + mutex_lock(&module_mutex);
> > > + /*
> > > + * We don't need to take a reference on the module here because we have
> > > + * the klp_mutex, which is also taken by the module notifier. This
> > > + * prevents any module from unloading until we release the klp_mutex.
> > > + */
> > > + obj->mod = find_module(obj->name);
> > > + mutex_unlock(&module_mutex);
> > > +
> > > + return !!obj->mod;
> >
> > I know that this is effective to return boolean here because
> > it handles also patch against the kernel core (vmlinux). But
> > the logic looks tricky. I would prefer a cleaner design and
> > use this function only to set obj->mod.
> >
> > I wanted to see how it would look like, so I will send a patch for
> > this in a separate mail.
>
> The patch is below. Of course, merge it into the original
> patch if you agree with the idea, please.
>
>
> >From 93eb9f9a25ad8aa0301e246f7685d3e787037566 Mon Sep 17 00:00:00 2001
> From: Petr Mladek <[email protected]>
> Date: Fri, 28 Nov 2014 15:32:27 +0100
> Subject: [PATCH 1/2] livepatch: clean up klp_find_object_module() usage
>
> The function klp_find_object_module() looks quite tricky. It has two effects:
> sets obj->mod and decides whether the module is available or not. The second
> effect is the tricky part because it handles also the code kernel code "vmlinux"
> and is not module related. It causes returning bool, and doing the crazy double
> negation.
>
> This patch tries to make a bit cleaner design:
>
> 1. klp_find_object_module() handles only obj->mod. It returns
> the pointer or NULL.
>
> 2. It modifies klp_enable_object() to do nothing when the related
> module has not been loaded yet.
>
> 3. The result is that the return value klp_find_object_module() is
> not used in the end.
>
> We lose a check for potential klp_enable_object() misuse but it makes the code
> easier. In fact, the check for unloaded module is rather long. We might want
> to make it easier using some extra flag or another state of the object.
> Such flag might be used for the check of misuse.
>
> Signed-off-by: Petr Mladek <[email protected]>

Hi,

I agree with the idea but actually don't like the implementation. I'll try
to propose few changes which would hopefully preserve the effect but make
the end result slightly better.

> ---
> kernel/livepatch/core.c | 14 +++++++-------
> 1 file changed, 7 insertions(+), 7 deletions(-)
>
> diff --git a/kernel/livepatch/core.c b/kernel/livepatch/core.c
> index 8e2e8cd242f5..9b1601729014 100644
> --- a/kernel/livepatch/core.c
> +++ b/kernel/livepatch/core.c
> @@ -40,10 +40,10 @@ static LIST_HEAD(klp_patches);
> static struct kobject *klp_root_kobj;
>
> /* sets obj->mod if object is not vmlinux and module is found */
> -static bool klp_find_object_module(struct klp_object *obj)
> +static struct module *klp_find_object_module(struct klp_object *obj)
> {
> if (!obj->name)
> - return 1;
> + return NULL;
>
> mutex_lock(&module_mutex);
> /*
> @@ -54,7 +54,7 @@ static bool klp_find_object_module(struct klp_object *obj)
> obj->mod = find_module(obj->name);
> mutex_unlock(&module_mutex);
>
> - return !!obj->mod;
> + return obj->mod;
> }

As we do not need the return value in the end, we could maybe drop it
completely, leading to change in if condition

if (!obj->name)
return;

> struct klp_find_arg {
> @@ -318,8 +318,9 @@ static int klp_enable_object(struct module *pmod, struct klp_object *obj)
> struct klp_func *func;
> int ret;
>
> - if (WARN_ON(!obj->mod && obj->name))
> - return -EINVAL;
> + /* nope when the patched module has not been loaded yet */
> + if (obj->name && !obj->mod)
> + return 0;

I have a problem with this one. In case the condition is true we do
nothing, return back and pretend that everything is alright (return 0).
I see that we would call klp_enable_object everytime in your proposal and
decide here whether we want to do something or not. But I think that we
should return some error and deal with it in the caller function. Thus
original WARN_ON should stay here.

> if (obj->relocs) {
> ret = klp_write_object_relocations(pmod, obj);
> @@ -401,8 +402,7 @@ static int __klp_enable_patch(struct klp_patch *patch)
> pr_notice("enabling patch '%s'\n", patch->mod->name);
>
> for (obj = patch->objs; obj->funcs; obj++) {
> - if (!klp_find_object_module(obj))
> - continue;
> + klp_find_object_module(obj);
> ret = klp_enable_object(patch->mod, obj);
> if (ret)
> goto unregister;

I propose this piece of code

for (obj = patch->objs; obj->funcs; obj++) {
klp_find_object_module(obj);
if (obj->name && !obj->mod)
continue;
ret = klp_enable_object(patch->mod, obj);
...
}

What do you think? Also it could pay off to define inline function for the
check. Somethink like klp_is_module_and_loaded...

Mira

2014-12-01 12:41:06

[permalink] [raw]

Subject: Re: [PATCH] livepatch: clean up klp_find_object_module() usage: was: Re: [PATCHv4 2/3] kernel: add support for live patching

On Mon 2014-12-01 13:08:50, Miroslav Benes wrote:
> On Fri, 28 Nov 2014, Petr Mladek wrote:
>
> > On Fri 2014-11-28 18:07:37, Petr Mladek wrote:
> > > On Tue 2014-11-25 11:15:08, Seth Jennings wrote:
> > > > This commit introduces code for the live patching core. It implements
> > > > an ftrace-based mechanism and kernel interface for doing live patching
> > > > of kernel and kernel module functions.
> >
> > [...]
> >
> > > > +/* sets obj->mod if object is not vmlinux and module is found */
> > > > +static bool klp_find_object_module(struct klp_object *obj)
> > > > +{
> > > > + if (!obj->name)
> > > > + return 1;
> > > > +
> > > > + mutex_lock(&module_mutex);
> > > > + /*
> > > > + * We don't need to take a reference on the module here because we have
> > > > + * the klp_mutex, which is also taken by the module notifier. This
> > > > + * prevents any module from unloading until we release the klp_mutex.
> > > > + */
> > > > + obj->mod = find_module(obj->name);
> > > > + mutex_unlock(&module_mutex);
> > > > +
> > > > + return !!obj->mod;
> > >
> > > I know that this is effective to return boolean here because
> > > it handles also patch against the kernel core (vmlinux). But
> > > the logic looks tricky. I would prefer a cleaner design and
> > > use this function only to set obj->mod.
> > >
> > > I wanted to see how it would look like, so I will send a patch for
> > > this in a separate mail.
> >
> > The patch is below. Of course, merge it into the original
> > patch if you agree with the idea, please.
> >
> >
> > >From 93eb9f9a25ad8aa0301e246f7685d3e787037566 Mon Sep 17 00:00:00 2001
> > From: Petr Mladek <[email protected]>
> > Date: Fri, 28 Nov 2014 15:32:27 +0100
> > Subject: [PATCH 1/2] livepatch: clean up klp_find_object_module() usage
> >
> > The function klp_find_object_module() looks quite tricky. It has two effects:
> > sets obj->mod and decides whether the module is available or not. The second
> > effect is the tricky part because it handles also the code kernel code "vmlinux"
> > and is not module related. It causes returning bool, and doing the crazy double
> > negation.
> >
> > This patch tries to make a bit cleaner design:
> >
> > 1. klp_find_object_module() handles only obj->mod. It returns
> > the pointer or NULL.
> >
> > 2. It modifies klp_enable_object() to do nothing when the related
> > module has not been loaded yet.
> >
> > 3. The result is that the return value klp_find_object_module() is
> > not used in the end.
> >
> > We lose a check for potential klp_enable_object() misuse but it makes the code
> > easier. In fact, the check for unloaded module is rather long. We might want
> > to make it easier using some extra flag or another state of the object.
> > Such flag might be used for the check of misuse.
> >
> > Signed-off-by: Petr Mladek <[email protected]>
>
> Hi,
>
> I agree with the idea but actually don't like the implementation. I'll try
> to propose few changes which would hopefully preserve the effect but make
> the end result slightly better.
>
> > ---
> > kernel/livepatch/core.c | 14 +++++++-------
> > 1 file changed, 7 insertions(+), 7 deletions(-)
> >
> > diff --git a/kernel/livepatch/core.c b/kernel/livepatch/core.c
> > index 8e2e8cd242f5..9b1601729014 100644
> > --- a/kernel/livepatch/core.c
> > +++ b/kernel/livepatch/core.c
> > @@ -40,10 +40,10 @@ static LIST_HEAD(klp_patches);
> > static struct kobject *klp_root_kobj;
> >
> > /* sets obj->mod if object is not vmlinux and module is found */
> > -static bool klp_find_object_module(struct klp_object *obj)
> > +static struct module *klp_find_object_module(struct klp_object *obj)
> > {
> > if (!obj->name)
> > - return 1;
> > + return NULL;
> >
> > mutex_lock(&module_mutex);
> > /*
> > @@ -54,7 +54,7 @@ static bool klp_find_object_module(struct klp_object *obj)
> > obj->mod = find_module(obj->name);
> > mutex_unlock(&module_mutex);
> >
> > - return !!obj->mod;
> > + return obj->mod;
> > }
>
> As we do not need the return value in the end, we could maybe drop it
> completely, leading to change in if condition
>
> if (!obj->name)
> return;

Sounds good.

> > struct klp_find_arg {
> > @@ -318,8 +318,9 @@ static int klp_enable_object(struct module *pmod, struct klp_object *obj)
> > struct klp_func *func;
> > int ret;
> >
> > - if (WARN_ON(!obj->mod && obj->name))
> > - return -EINVAL;
> > + /* nope when the patched module has not been loaded yet */
> > + if (obj->name && !obj->mod)
> > + return 0;
>
> I have a problem with this one. In case the condition is true we do
> nothing, return back and pretend that everything is alright (return 0).
> I see that we would call klp_enable_object everytime in your proposal and
> decide here whether we want to do something or not. But I think that we
> should return some error and deal with it in the caller function. Thus
> original WARN_ON should stay here.

I agree.

> > if (obj->relocs) {
> > ret = klp_write_object_relocations(pmod, obj);
> > @@ -401,8 +402,7 @@ static int __klp_enable_patch(struct klp_patch *patch)
> > pr_notice("enabling patch '%s'\n", patch->mod->name);
> >
> > for (obj = patch->objs; obj->funcs; obj++) {
> > - if (!klp_find_object_module(obj))
> > - continue;
> > + klp_find_object_module(obj);
> > ret = klp_enable_object(patch->mod, obj);
> > if (ret)
> > goto unregister;
>
> I propose this piece of code
>
> for (obj = patch->objs; obj->funcs; obj++) {
> klp_find_object_module(obj);
> if (obj->name && !obj->mod)
> continue;
> ret = klp_enable_object(patch->mod, obj);
> ...
> }

I like this change, especially if we replace the condition with a
macro. It keeps klp_find_object_module() without the side effect
and it keeps the warning in klp_enable_object().

> What do you think? Also it could pay off to define inline function for the
> check. Somethink like klp_is_module_and_loaded...

klp_is_object_loaded() is easier and still clear. Also we might want
to add a macro klp_is_module(obj).

Best Regards,
Petr

2014-12-01 13:31:27

[permalink] [raw]

Subject: Re: [PATCHv4 2/3] kernel: add support for live patching

On Wed, 26 Nov 2014, Josh Poimboeuf wrote:

> Hi Miroslav,
>
> Just addressing one of your comments below. I'll let Seth respond to
> the others :-)
>
> On Wed, Nov 26, 2014 at 03:19:17PM +0100, Miroslav Benes wrote:
> > > +/**
> > > + * struct klp_func - function structure for live patching
> > > + * @old_name: name of the function to be patched
> > > + * @new_func: pointer to the patched function code
> > > + * @old_addr: a hint conveying at what address the old function
> > > + * can be found (optional, vmlinux patches only)
> > > + */
> > > +struct klp_func {
> > > + /* external */
> > > + const char *old_name;
> > > + void *new_func;
> > > + /*
> > > + * The old_addr field is optional and can be used to resolve
> > > + * duplicate symbol names in the vmlinux object. If this
> > > + * information is not present, the symbol is located by name
> > > + * with kallsyms. If the name is not unique and old_addr is
> > > + * not provided, the patch application fails as there is no
> > > + * way to resolve the ambiguity.
> > > + */
> > > + unsigned long old_addr;
> >
> > I wonder if we really need old_addr as an external field. I assume that
> > userspace tool in kpatch use it as a "hint" for kernel part and thus
> > kallsyms is not needed there (and it solves ambiguity problem as well).
> > But I am not sure if it is gonna be the same in upstream. When kernel is
> > randomized (CONFIG_RANDOMIZE_BASE is set to 'y', though default is 'n')
> > old_addr is not usable (and we throw it away in the code). Without
> > old_addr being set by the user we could spare some of code (calls to
> > klp_verify_vmlinux_symbol and such).
>
> Even with CONFIG_RANDOMIZE_BASE, the function offsets will be the same
> regardless of the base address. So we could still use old_addr to
> determine the offset.
>
> > So the question is whether future userspace tool in upstream would need it
> > and would use it. Please note that I do not mean it as a kpatch or kgraft
> > way to do things, I'm just not sure about old_addr being "public" and want
> > do discuss the options.
> >
> > The ambiguity of symbols was discussed in some other thread in lkml in
> > october (I guess) with no conclusion IIRC...
>
> We need to resolve ambiguity somehow, and old_addr is a way to do that.
> Do you have any other ideas?

Unfortunately I don't.

But similarly we don't deal with ambiguity in modules either. And it is
(at least theoretically) possible. Two static functions of the same name
in two different .c files which the final module is linked from. You have
to use kallsyms and it would get confused. Maybe this sounds odd but it
could happen.

Thus the old_addr value is not general protection (as modules are still
affected) and it is questionable whether the user should use it.

I do not have strong opinion on this and if no one else shares my
thoughts, I am not against.

Mira

2014-12-01 17:07:56

[permalink] [raw]

Subject: Re: [PATCHv4 2/3] kernel: add support for live patching

On Mon, Dec 01, 2014 at 02:31:35PM +0100, Miroslav Benes wrote:
> On Wed, 26 Nov 2014, Josh Poimboeuf wrote:
>
> > Hi Miroslav,
> >
> > Just addressing one of your comments below. I'll let Seth respond to
> > the others :-)
> >
> > On Wed, Nov 26, 2014 at 03:19:17PM +0100, Miroslav Benes wrote:
> > > > +/**
> > > > + * struct klp_func - function structure for live patching
> > > > + * @old_name: name of the function to be patched
> > > > + * @new_func: pointer to the patched function code
> > > > + * @old_addr: a hint conveying at what address the old function
> > > > + * can be found (optional, vmlinux patches only)
> > > > + */
> > > > +struct klp_func {
> > > > + /* external */
> > > > + const char *old_name;
> > > > + void *new_func;
> > > > + /*
> > > > + * The old_addr field is optional and can be used to resolve
> > > > + * duplicate symbol names in the vmlinux object. If this
> > > > + * information is not present, the symbol is located by name
> > > > + * with kallsyms. If the name is not unique and old_addr is
> > > > + * not provided, the patch application fails as there is no
> > > > + * way to resolve the ambiguity.
> > > > + */
> > > > + unsigned long old_addr;
> > >
> > > I wonder if we really need old_addr as an external field. I assume that
> > > userspace tool in kpatch use it as a "hint" for kernel part and thus
> > > kallsyms is not needed there (and it solves ambiguity problem as well).
> > > But I am not sure if it is gonna be the same in upstream. When kernel is
> > > randomized (CONFIG_RANDOMIZE_BASE is set to 'y', though default is 'n')
> > > old_addr is not usable (and we throw it away in the code). Without
> > > old_addr being set by the user we could spare some of code (calls to
> > > klp_verify_vmlinux_symbol and such).
> >
> > Even with CONFIG_RANDOMIZE_BASE, the function offsets will be the same
> > regardless of the base address. So we could still use old_addr to
> > determine the offset.
> >
> > > So the question is whether future userspace tool in upstream would need it
> > > and would use it. Please note that I do not mean it as a kpatch or kgraft
> > > way to do things, I'm just not sure about old_addr being "public" and want
> > > do discuss the options.
> > >
> > > The ambiguity of symbols was discussed in some other thread in lkml in
> > > october (I guess) with no conclusion IIRC...
> >
> > We need to resolve ambiguity somehow, and old_addr is a way to do that.
> > Do you have any other ideas?
>
> Unfortunately I don't.
>
> But similarly we don't deal with ambiguity in modules either. And it is
> (at least theoretically) possible. Two static functions of the same name
> in two different .c files which the final module is linked from. You have
> to use kallsyms and it would get confused. Maybe this sounds odd but it
> could happen.

True, this is a remote possibility, but we haven't run into this issue
yet. If it becomes a problem, we can try to come up with another way to
resolve duplicates.

Here's one idea: since the symbols are always listed in the same order
in kallsyms (per-object), instead of old_addr we could have sym_idx. A
sym_idx of 2 could mean "I want the 2nd occurence of foo in the object's
kallsyms list".

However I'd rather keep our current old_addr scheme for now, since it's
what we have implemented already. And there are plenty of more
important things we need to do first.

> Thus the old_addr value is not general protection (as modules are still
> affected) and it is questionable whether the user should use it.

It's not really protection, since even if you don't specify old_addr and
you rely on kallsyms, klp_find_symbol will return an error if there are
any duplicates.

It's really just a way to increase the size of the set of functions
which can be patched (duplicately named functions).

We also rely on something similar for relocations: klp_reloc.src. It's
even more important there, since duplicately named static objects are
more common than duplicately named functions.

> I do not have strong opinion on this and if no one else shares my
> thoughts, I am not against.
>
> Mira

--
Josh

2014-12-01 17:12:05

[permalink] [raw]

Subject: Re: [PATCHv4 3/3] samples: add sample live patching module

On Thu, Nov 27, 2014 at 06:05:13PM +0100, Petr Mladek wrote:
> On Tue 2014-11-25 11:15:09, Seth Jennings wrote:
> > Add a sample live patching module.
> >
> > Signed-off-by: Seth Jennings <[email protected]>
> > ---
> > MAINTAINERS | 1 +
> > samples/Kconfig | 7 +++
> > samples/livepatch/Makefile | 1 +
> > samples/livepatch/livepatch-sample.c | 87 ++++++++++++++++++++++++++++++++++++
> > 4 files changed, 96 insertions(+)
> > create mode 100644 samples/livepatch/Makefile
> > create mode 100644 samples/livepatch/livepatch-sample.c
>
> [...]
>
> > diff --git a/samples/livepatch/Makefile b/samples/livepatch/Makefile
> > new file mode 100644
> > index 0000000..7f1cdc1
> > --- /dev/null
> > +++ b/samples/livepatch/Makefile
> > @@ -0,0 +1 @@
> > +obj-$(CONFIG_SAMPLE_LIVE_PATCHING) += livepatch-sample.o
> > diff --git a/samples/livepatch/livepatch-sample.c b/samples/livepatch/livepatch-sample.c
> > new file mode 100644
> > index 0000000..21f159d
>
> The Makefile is ignored because there is missing:
>
>
> diff --git a/samples/Makefile b/samples/Makefile
> index 1a60c62e2045..f00257bcc5a7 100644
> --- a/samples/Makefile
> +++ b/samples/Makefile
> @@ -1,4 +1,4 @@
> # Makefile for Linux samples code
>
> -obj-$(CONFIG_SAMPLES) += kobject/ kprobes/ trace_events/ \
> +obj-$(CONFIG_SAMPLES) += kobject/ kprobes/ trace_events/ livepatch/ \
> hw_breakpoint/ kfifo/ kdb/ hidraw/ rpmsg/ seccomp/

Applied, thanks!

Seth

>
>
> Best Regards,
> Petr
> --
> To unsubscribe from this list: send the line "unsubscribe live-patching" in
> the body of a message to [email protected]
> More majordomo info at http://vger.kernel.org/majordomo-info.html

2014-12-02 12:23:55

[permalink] [raw]

Subject: Re: [PATCHv4 2/3] kernel: add support for live patching

On Mon, 1 Dec 2014, Josh Poimboeuf wrote:

> On Mon, Dec 01, 2014 at 02:31:35PM +0100, Miroslav Benes wrote:
> > On Wed, 26 Nov 2014, Josh Poimboeuf wrote:
> >
> > > Hi Miroslav,
> > >
> > > Just addressing one of your comments below. I'll let Seth respond to
> > > the others :-)
> > >
> > > On Wed, Nov 26, 2014 at 03:19:17PM +0100, Miroslav Benes wrote:
> > > > > +/**
> > > > > + * struct klp_func - function structure for live patching
> > > > > + * @old_name: name of the function to be patched
> > > > > + * @new_func: pointer to the patched function code
> > > > > + * @old_addr: a hint conveying at what address the old function
> > > > > + * can be found (optional, vmlinux patches only)
> > > > > + */
> > > > > +struct klp_func {
> > > > > + /* external */
> > > > > + const char *old_name;
> > > > > + void *new_func;
> > > > > + /*
> > > > > + * The old_addr field is optional and can be used to resolve
> > > > > + * duplicate symbol names in the vmlinux object. If this
> > > > > + * information is not present, the symbol is located by name
> > > > > + * with kallsyms. If the name is not unique and old_addr is
> > > > > + * not provided, the patch application fails as there is no
> > > > > + * way to resolve the ambiguity.
> > > > > + */
> > > > > + unsigned long old_addr;
> > > >
> > > > I wonder if we really need old_addr as an external field. I assume that
> > > > userspace tool in kpatch use it as a "hint" for kernel part and thus
> > > > kallsyms is not needed there (and it solves ambiguity problem as well).
> > > > But I am not sure if it is gonna be the same in upstream. When kernel is
> > > > randomized (CONFIG_RANDOMIZE_BASE is set to 'y', though default is 'n')
> > > > old_addr is not usable (and we throw it away in the code). Without
> > > > old_addr being set by the user we could spare some of code (calls to
> > > > klp_verify_vmlinux_symbol and such).
> > >
> > > Even with CONFIG_RANDOMIZE_BASE, the function offsets will be the same
> > > regardless of the base address. So we could still use old_addr to
> > > determine the offset.
> > >
> > > > So the question is whether future userspace tool in upstream would need it
> > > > and would use it. Please note that I do not mean it as a kpatch or kgraft
> > > > way to do things, I'm just not sure about old_addr being "public" and want
> > > > do discuss the options.
> > > >
> > > > The ambiguity of symbols was discussed in some other thread in lkml in
> > > > october (I guess) with no conclusion IIRC...
> > >
> > > We need to resolve ambiguity somehow, and old_addr is a way to do that.
> > > Do you have any other ideas?
> >
> > Unfortunately I don't.
> >
> > But similarly we don't deal with ambiguity in modules either. And it is
> > (at least theoretically) possible. Two static functions of the same name
> > in two different .c files which the final module is linked from. You have
> > to use kallsyms and it would get confused. Maybe this sounds odd but it
> > could happen.
>
> True, this is a remote possibility, but we haven't run into this issue
> yet. If it becomes a problem, we can try to come up with another way to
> resolve duplicates.
>
> Here's one idea: since the symbols are always listed in the same order
> in kallsyms (per-object), instead of old_addr we could have sym_idx. A
> sym_idx of 2 could mean "I want the 2nd occurence of foo in the object's
> kallsyms list".

This sounds crazy and I hope we won't be forced to implement it after all
:)

> However I'd rather keep our current old_addr scheme for now, since it's
> what we have implemented already. And there are plenty of more
> important things we need to do first.

Agreed

> > Thus the old_addr value is not general protection (as modules are still
> > affected) and it is questionable whether the user should use it.
>
> It's not really protection, since even if you don't specify old_addr and
> you rely on kallsyms, klp_find_symbol will return an error if there are
> any duplicates.
>
> It's really just a way to increase the size of the set of functions
> which can be patched (duplicately named functions).
>
> We also rely on something similar for relocations: klp_reloc.src. It's
> even more important there, since duplicately named static objects are
> more common than duplicately named functions.

I understand. So let's settle for leaving it as it is.

Thanks
Mira

> > I do not have strong opinion on this and if no one else shares my
> > thoughts, I am not against.
> >
> > Mira
>
> --
> Josh
>

--
Miroslav Benes
SUSE Labs

2014-12-03 10:00:13