On Thu, 20 Nov 2014, 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. > > Signed-off-by: Seth Jennings <sjenning@xxxxxxxxxx> I think this is getting really close, which is awesome. A few rather minor nits below. [ ... snip ... ] > diff --git a/arch/x86/include/asm/livepatch.h b/arch/x86/include/asm/livepatch.h > new file mode 100644 > index 0000000..2ed86ec > --- /dev/null > +++ b/arch/x86/include/asm/livepatch.h > @@ -0,0 +1,37 @@ > +/* > + * livepatch.h - x86-specific Kernel Live Patching Core > + * > + * Copyright (C) 2014 Seth Jennings <sjenning@xxxxxxxxxx> > + * > + * 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 > +extern int klp_write_module_reloc(struct module *mod, unsigned long type, > + unsigned long loc, unsigned long value); > + > +#else > +static int klp_write_module_reloc(struct module *mod, unsigned long type, static inline? [ ... snip ... ] > --- /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 We have to refuse to build on x86_64 if the compiler doesn't support fentry. mcount is not really usable (well, it would be possible to use it, be the obstacles are too big to care). Something like [1] should be applicable here as well I believe. [1] https://git.kernel.org/cgit/linux/kernel/git/jirislaby/kgraft.git/commit/?h=kgraft&id=bd4bc097c72937d18036f1312a4d79ed0bea9991 [ ... snip ... ] > --- /dev/null > +++ b/kernel/livepatch/core.c > @@ -0,0 +1,828 @@ > +/* > + * core.c - Kernel Live Patching Core > + * > + * Copyright (C) 2014 Seth Jennings <sjenning@xxxxxxxxxx> > + * > + * 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> > + > +/************************************* > + * Core structures > + ************************************/ I don't personally find such markers (especially with all the '*'s) too tasteful, and I don't recall ever seeing this being common pattern used in the kernel code ... ? > +static DEFINE_MUTEX(klp_mutex); > +static LIST_HEAD(klp_patches); > + > +/******************************************* > + * Helpers > + *******************************************/ > + > +/* sets obj->mod if object is not vmlinux and module is found */ > +static bool klp_find_object_module(struct klp_object *obj) > +{ > + if (!strcmp(obj->name, "vmlinux")) > + return 1; Rather a matter of taste again -- I personally would prefer "obj->name == NULL" to be the condition identifying core kernel code text. You can't really forbid any lunetic out there calling his kernel module "vmlinux", right? :) [ ... snip ... ] > +/*********************************** > + * ftrace registration > + **********************************/ > + > +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 || func->state != LPC_DISABLED)) > + return -EINVAL; If the WARN_ON triggers, there is no good way to find out which of the two conditions triggered it. [ ... snip ... ] > +static int klp_init_patch(struct klp_patch *patch) > +{ > + int ret; > + > + mutex_lock(&klp_mutex); > + > + /* init */ > + patch->state = LPC_DISABLED; > + > + /* sysfs */ > + ret = kobject_init_and_add(&patch->kobj, &klp_ktype_patch, > + klp_root_kobj, patch->mod->name); > + if (ret) > + return ret; klp_mutex is leaked locked here. > + > + /* create objects */ > + ret = klp_init_objects(patch); > + if (ret) { > + kobject_put(&patch->kobj); > + return ret; And here as well. All in all, this is looking very good to me. I think we are really close to having a code that all the parties would agree with. Thanks everybody, -- Jiri Kosina SUSE Labs -- To unsubscribe from this list: send the line "unsubscribe live-patching" in the body of a message to majordomo@xxxxxxxxxxxxxxx More majordomo info at http://vger.kernel.org/majordomo-info.html
