| lists.openwall.net | lists / announce owl-users owl-dev john-users john-dev passwdqc-users yescrypt popa3d-users / oss-security kernel-hardening musl sabotage tlsify passwords / crypt-dev xvendor / Bugtraq Full-Disclosure linux-kernel linux-netdev linux-ext4 linux-hardening linux-cve-announce PHC | |
|
Open Source and information security mailing list archives
| ||
|
Date: Fri, 7 Nov 2014 18:39:03 +0100
From: Petr Mladek <pmladek@...e.cz>
To: Seth Jennings <sjenning@...hat.com>
Cc: Josh Poimboeuf <jpoimboe@...hat.com>,
Jiri Kosina <jkosina@...e.cz>,
Vojtech Pavlik <vojtech@...e.cz>,
Steven Rostedt <rostedt@...dmis.org>,
live-patching@...r.kernel.org, kpatch@...hat.com,
linux-kernel@...r.kernel.org
Subject: more patches for the same func: was Re: [PATCH 2/2] kernel: add
support for live patching
On Thu 2014-11-06 08:39:08, 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.
[...]
> +static int lpc_enable_func(struct lpc_func *func)
> +{
> + int ret;
> +
> + BUG_ON(!func->old_addr);
> + BUG_ON(func->state != DISABLED);
> + ret = ftrace_set_filter_ip(&func->fops, func->old_addr, 0, 0);
> + if (ret) {
> + pr_err("failed to set ftrace filter for function '%s' (%d)\n",
> + func->old_name, ret);
> + return ret;
> + }
> + ret = register_ftrace_function(&func->fops);
> + if (ret) {
> + pr_err("failed to register ftrace handler for function '%s' (%d)\n",
> + func->old_name, ret);
> + ftrace_set_filter_ip(&func->fops, func->old_addr, 1, 0);
> + } else
> + func->state = ENABLED;
> +
> + return ret;
> +}
> +
[...]
> +/* caller must ensure that obj->mod is set if object is a module */
> +static int lpc_enable_object(struct module *pmod, struct lpc_object *obj)
> +{
> + struct lpc_func *func;
> + int ret;
> +
> + if (obj->mod && !try_module_get(obj->mod))
> + return -ENODEV;
> +
> + if (obj->dynrelas) {
> + ret = lpc_write_object_relocations(pmod, obj);
> + if (ret)
> + goto unregister;
> + }
> + list_for_each_entry(func, &obj->funcs, list) {
> + ret = lpc_find_verify_func_addr(func, obj->name);
> + if (ret)
> + goto unregister;
> +
> + ret = lpc_enable_func(func);
> + if (ret)
> + goto unregister;
> + }
> + obj->state = ENABLED;
> +
> + return 0;
> +unregister:
> + WARN_ON(lpc_unregister_object(obj));
> + return ret;
> +}
> +
> +/******************************
> + * enable/disable
> + ******************************/
> +
> +/* must be called with lpc_mutex held */
> +static struct lpc_patch *lpc_find_patch(struct lp_patch *userpatch)
> +{
> + struct lpc_patch *patch;
> +
> + list_for_each_entry(patch, &lpc_patches, list)
> + if (patch->userpatch == userpatch)
> + return patch;
> +
> + return NULL;
> +}
[...]
> +
> +/* must be called with lpc_mutex held */
> +static int lpc_enable_patch(struct lpc_patch *patch)
> +{
> + struct lpc_object *obj;
> + int ret;
> +
> + BUG_ON(patch->state != DISABLED);
> +
> + pr_notice_once("tainting kernel with TAINT_LIVEPATCH\n");
> + add_taint(TAINT_LIVEPATCH, LOCKDEP_STILL_OK);
> +
> + pr_notice("enabling patch '%s'\n", patch->mod->name);
> +
> + list_for_each_entry(obj, &patch->objs, list) {
> + if (!is_object_loaded(obj))
> + continue;
> + ret = lpc_enable_object(patch->mod, obj);
> + if (ret)
> + goto unregister;
> + }
> + patch->state = ENABLED;
> + return 0;
> +
> +unregister:
> + WARN_ON(lpc_disable_patch(patch));
> + return ret;
> +}
> +
> +int lp_enable_patch(struct lp_patch *userpatch)
> +{
> + struct lpc_patch *patch;
> + int ret;
> +
> + down(&lpc_mutex);
> + patch = lpc_find_patch(userpatch);
> + if (!patch) {
> + ret = -ENODEV;
> + goto out;
> + }
> + ret = lpc_enable_patch(patch);
> +out:
> + up(&lpc_mutex);
> + return ret;
> +}
> +EXPORT_SYMBOL_GPL(lp_enable_patch);
AFAIK, this does not handle correctly the situation when there
are more patches for the same symbol. IMHO, the first registered
ftrace function wins. It means that later patches are ignored.
In kGraft, we detect this situation and do the following:
add_new_ftrace_function()
/* old one still might be used at this stage */
if (old_function)
remove_old_ftrace_function();
/* the new one is used from now on */
Similar problem is when a patch is disabled. We need to know
if it was actually used. If not, we are done. If it is active,
we need to look if there is an older patch for the the same
symbol and enable the other ftrace function instead.
Best Regards,
Petr
PS: We should probably decide on the used structures before we start
coding fixes for this particular problems. I have similar concern about
the complexity as my colleagues have. But I need to think more about
it. Let's discuss it in the other thread.
--
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