Message-ID: <20141121164027.GB11339@cerebellum.variantweb.net>
Date:	Fri, 21 Nov 2014 10:40:27 -0600
From:	Seth Jennings <sjenning@...hat.com>
To:	Jiri Kosina <jkosina@...e.cz>
Cc:	Josh Poimboeuf <jpoimboe@...hat.com>,
	Vojtech Pavlik <vojtech@...e.cz>,
	Steven Rostedt <rostedt@...dmis.org>,
	Petr Mladek <pmladek@...e.cz>, Miroslav Benes <mbenes@...e.cz>,
	Christoph Hellwig <hch@...radead.org>,
	Greg KH <gregkh@...uxfoundation.org>,
	Andy Lutomirski <luto@...capital.net>,
	Masami Hiramatsu <masami.hiramatsu.pt@...achi.com>,
	live-patching@...r.kernel.org, x86@...nel.org, kpatch@...hat.com,
	linux-kernel@...r.kernel.org
Subject: Re: [PATCHv3 2/3] kernel: add support for live patching

On Fri, Nov 21, 2014 at 01:22:33AM +0100, Jiri Kosina wrote:
> 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@...hat.com>
> 
> 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@...hat.com>
> > + *
> > + * 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?

I think the practice is to let the compiler handle inline determination
unless you are sure that the compiler isn't inlining something you think
it should.

All other changes are accepted and queued for v4.

Thanks,
Seth

> 
> [ ... 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@...hat.com>
> > + *
> > + * 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 linux-kernel" in
the body of a message to majordomo@...r.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html
Please read the FAQ at  http://www.tux.org/lkml/

Powered by blists - more mailing lists