Message-ID: <alpine.LNX.2.00.1411210102280.23174@pobox.suse.cz>
Date:	Fri, 21 Nov 2014 01:22:33 +0100 (CET)
From:	Jiri Kosina <jkosina@...e.cz>
To:	Seth Jennings <sjenning@...hat.com>
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 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?

[ ... 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