Return-Path: Received: (majordomo@vger.kernel.org) by vger.kernel.org via listexpand id S1757014AbaKUAWd (ORCPT ); Thu, 20 Nov 2014 19:22:33 -0500 Received: from cantor2.suse.de ([195.135.220.15]:42581 "EHLO mx2.suse.de" rhost-flags-OK-OK-OK-OK) by vger.kernel.org with ESMTP id S1755490AbaKUAWc (ORCPT ); Thu, 20 Nov 2014 19:22:32 -0500 Date: Fri, 21 Nov 2014 01:22:33 +0100 (CET) From: Jiri Kosina To: Seth Jennings cc: Josh Poimboeuf , Vojtech Pavlik , Steven Rostedt , Petr Mladek , Miroslav Benes , Christoph Hellwig , Greg KH , Andy Lutomirski , Masami Hiramatsu , live-patching@vger.kernel.org, x86@kernel.org, kpatch@redhat.com, linux-kernel@vger.kernel.org Subject: Re: [PATCHv3 2/3] kernel: add support for live patching In-Reply-To: <1416522580-5593-3-git-send-email-sjenning@redhat.com> Message-ID: References: <1416522580-5593-1-git-send-email-sjenning@redhat.com> <1416522580-5593-3-git-send-email-sjenning@redhat.com> User-Agent: Alpine 2.00 (LNX 1167 2008-08-23) MIME-Version: 1.0 Content-Type: TEXT/PLAIN; charset=US-ASCII Sender: linux-kernel-owner@vger.kernel.org List-ID: X-Mailing-List: linux-kernel@vger.kernel.org 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 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 > + * > + * 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 . > + */ > + > +#ifndef _ASM_X86_LIVEPATCH_H > +#define _ASM_X86_LIVEPATCH_H > + > +#include > + > +#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 > + * > + * 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 . > + */ > + > +#define pr_fmt(fmt) KBUILD_MODNAME ": " fmt > + > +#include > +#include > +#include > +#include > +#include > +#include > +#include > +#include > + > +/************************************* > + * 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 linux-kernel" in the body of a message to majordomo@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html Please read the FAQ at http://www.tux.org/lkml/