linux-kernel - Re: [PATCHv6 2/3] kernel: add support for live patching

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
Hash Suite: Windows password security audit tool. GUI, reports in PDF.
[<prev] [next>] [<thread-prev] [day] [month] [year] [list]
Message-ID: <20141213200615.GA21557@treble.redhat.com>
Date:	Sat, 13 Dec 2014 14:06:15 -0600
From:	Josh Poimboeuf <jpoimboe@...hat.com>
To:	Miroslav Benes <mbenes@...e.cz>
Cc:	Seth Jennings <sjenning@...hat.com>, Jiri Kosina <jkosina@...e.cz>,
	Vojtech Pavlik <vojtech@...e.cz>,
	Steven Rostedt <rostedt@...dmis.org>,
	Petr Mladek <pmladek@...e.cz>,
	Masami Hiramatsu <masami.hiramatsu.pt@...achi.com>,
	Jiri Slaby <jslaby@...e.cz>,
	Christoph Hellwig <hch@...radead.org>,
	Greg KH <gregkh@...uxfoundation.org>,
	Andy Lutomirski <luto@...capital.net>,
	live-patching@...r.kernel.org, x86@...nel.org, kpatch@...hat.com,
	linux-kernel@...r.kernel.org
Subject: Re: [PATCHv6 2/3] kernel: add support for live patching

On Fri, Dec 12, 2014 at 05:58:19PM +0100, Miroslav Benes wrote:
> 
> Hi, 
> 
> I think we are really close (or I hope so). I found few suspicious things 
> or nitpicks though. They might have applied also to v5, but I didn't 
> manage to look at that. Sorry about that.
> 
> On Wed, 10 Dec 2014, Josh Poimboeuf wrote:
> 
> > +/* klp_mutex must be held by caller */
> > +static bool klp_patch_is_registered(struct klp_patch *patch)
> 
> Maybe klp_is_patch_registered is more appropriate name (consistent with 
> other predicates in the file).

Ok.

> > +static int klp_disable_func(struct klp_func *func)
> > +{
> > +	int ret;
> > +
> > +	if (WARN_ON(func->state != KLP_ENABLED))
> > +		return -EINVAL;
> > +
> > +	if (WARN_ON(!func->old_addr))
> > +		return -EINVAL;
> > +
> > +	ret = unregister_ftrace_function(func->fops);
> > +	if (ret) {
> > +		pr_err("failed to unregister ftrace handler for function '%s' (%d)\n",
> > +		       func->old_name, ret);
> > +		return ret;
> > +	}
> > +
> > +	ret = ftrace_set_filter_ip(func->fops, func->old_addr, 1, 0);
> > +	if (ret)
> > +		pr_warn("function unregister succeeded but failed to clear the filter\n");
> > +
> > +	func->state = KLP_DISABLED;
> > +
> > +	return 0;
> > +}
> > +
> > +static int klp_enable_func(struct klp_func *func)
> > +{
> > +	int ret;
> > +
> > +	if (WARN_ON(!func->old_addr))
> > +		return -EINVAL;
> > +
> > +	if (WARN_ON(func->state != KLP_DISABLED))
> > +		return -EINVAL;
> > +
> > +	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 = KLP_ENABLED;
> > +	}
> > +
> > +	return ret;
> > +}
> 
> Just to be sure about our policy. We want to be stricter during enabling 
> than in disabling process. Is that correct? Otherwise there is 
> inconsistency in pr_* macros and return values. Also fops could be 
> hypothetically registered back when ftrace_set_filter_ip fails in 
> klp_disable_func. I just want to be sure that we didn't overlook 
> anything...

The asymmetry in the enable/disable error handling is intentional.  In
klp_disable_func(), a ftrace_set_filter_ip() failure isn't a fatal
condition because we've already unregistered the fops and thus removed
the patch.

> > +static int klp_init_func(struct klp_object *obj, struct klp_func *func)
> > +{
> > +	struct ftrace_ops *ops;
> > +	int ret;
> > +
> > +	func->state = KLP_DISABLED;
> > +
> > +	ops = kzalloc(sizeof(*ops), GFP_KERNEL);
> > +	if (!ops)
> > +		ret = -ENOMEM;
> 
> There should be return -ENOMEM.

Agreed.

> > +static int klp_init_object(struct klp_patch *patch, struct klp_object *obj)
> > +{
> > +	struct klp_func *func;
> > +	int ret;
> > +	const char *name;
> > +
> > +	if (!obj->funcs)
> > +		return -EINVAL;
> > +
> > +	obj->state = KLP_DISABLED;
> > +
> > +	klp_find_object_module(obj);
> > +
> > +	name = klp_is_module(obj) ? obj->name : "vmlinux";
> > +	obj->kobj = kobject_create_and_add(name, &patch->kobj);
> > +	if (!obj->kobj)
> > +		return -ENOMEM;
> > +
> > +	for (func = obj->funcs; func->old_name; func++) {
> > +		ret = klp_init_func(obj, func);
> > +		if (ret)
> > +			goto free;
> > +	}
> > +
> > +	if (klp_is_object_loaded(obj)) {
> > +		ret = klp_init_object_loaded(patch, obj);
> > +		if (ret)
> > +			goto free;
> > +	}
> > +
> > +	return 0;
> > +
> > +free:
> > +	klp_free_funcs_limited(obj, func);
> > +	return ret;
> > +}
> 
> Shouldn't we call kobject_put(obj->kobj) in free branch? If I am not wrong 
> it is not freed anywhere else. We free only already initialized functions 
> and already initialized objects later in klp_init_patch, but not the 
> kobject of the currently failing object.

Agreed.

> And that is everything. I like it, it has improved a lot. I hope that 
> there are no other problems. I am getting blind looking at it all the 
> time :)

Thanks!  I'll send out the next patch set soon, maybe Monday.

-- 
Josh
--
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/