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

From: Jiri Kosina
Date: Thu Nov 20 2014 - 19:22:36 EST


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