linux-kernel - Re: [PATCH v2] livepatch: old_name.number scheme in livepatch sysfs directory

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] [thread-next>] [day] [month] [year] [list]

Message-ID: <20151103165058.GM27488@treble.redhat.com>
Date:	Tue, 3 Nov 2015 10:50:58 -0600
From:	Josh Poimboeuf <jpoimboe@...hat.com>
To:	Miroslav Benes <mbenes@...e.cz>
Cc:	Chris J Arges <chris.j.arges@...onical.com>,
	live-patching@...r.kernel.org, jeyu@...hat.com,
	Seth Jennings <sjenning@...hat.com>,
	Jiri Kosina <jikos@...nel.org>,
	Vojtech Pavlik <vojtech@...e.com>, linux-api@...r.kernel.org,
	linux-kernel@...r.kernel.org
Subject: Re: [PATCH v2] livepatch: old_name.number scheme in livepatch sysfs
 directory

On Tue, Nov 03, 2015 at 05:09:48PM +0100, Miroslav Benes wrote:
> On Tue, 3 Nov 2015, Josh Poimboeuf wrote:
> 
> > On Tue, Nov 03, 2015 at 11:52:08AM +0100, Miroslav Benes wrote:
> > > On Mon, 2 Nov 2015, Chris J Arges wrote:
> > > 
> > > [...]
> > > 
> > > > +static int klp_get_func_pos_callback(void *data, const char *name,
> > > > +				      struct module *mod, unsigned long addr)
> > > > +{
> > > > +	struct klp_find_arg *args = data;
> > > > +
> > > > +	if ((mod && !args->objname) || (!mod && args->objname))
> > > > +		return 0;
> > > > +
> > > > +	if (strcmp(args->name, name))
> > > > +		return 0;
> > > > +
> > > > +	if (args->objname && strcmp(args->objname, mod->name))
> > > > +		return 0;
> > > > +
> > > > +	/* on address match, return 1 to break kallsyms_on_each_symbol loop */
> > > > +	if (args->addr == addr)
> > > > +		return 1;
> > > > +
> > > > +	/* if we don't match addr, count instance of named symbol */
> > > > +	args->count++;
> > > > +
> > > > +	return 0;
> > > > +}
> > > > +
> > > > +static int klp_get_func_pos(struct klp_object *obj, struct klp_func *func)
> > > > +{
> > > > +	struct klp_find_arg args = {
> > > > +		.objname = obj->name,
> > > > +		.name = func->old_name,
> > > > +		.addr = func->old_addr,
> > > > +		.count = 0,
> > > > +	};
> > > > +
> > > > +	mutex_lock(&module_mutex);
> > > > +	kallsyms_on_each_symbol(klp_get_func_pos_callback, &args);
> > > > +	mutex_unlock(&module_mutex);
> > > > +
> > > > +	return args.count;
> > > > +}
> > > > +
> > > >  static int klp_init_func(struct klp_object *obj, struct klp_func *func)
> > > >  {
> > > >  	INIT_LIST_HEAD(&func->stack_node);
> > > >  	func->state = KLP_DISABLED;
> > > >  
> > > >  	return kobject_init_and_add(&func->kobj, &klp_ktype_func,
> > > > -				    &obj->kobj, "%s", func->old_name);
> > > > +				    &obj->kobj, "%s,%d", func->old_name,
> > > > +				    klp_get_func_pos(obj, func));
> > > >  }
> > > 
> > > There is a problem which I missed before. klp_init_func() is called before 
> > > klp_find_verify_func_addr() in klp_init_object(). This means that 
> > > func->old_addr is either not verified yet or worse it is still 0. This 
> > > means that klp_get_func_pos_callback() never returns 1 and is thus called 
> > > on each symbol. So if you for example patched cmdline_proc_show the 
> > > resulting directory in sysfs would be called cmdline_proc_show,1 because 
> > > addr is never matched. Had old_addr been specified the name would have 
> > > been probably correct, but not for sure.
> > > 
> > > This should be fixed as well.
> > 
> > Even worse, klp_init_func() can be called even if the object hasn't been
> > loaded.  In that case there's no way to know what the value of n is, and
> > therefore no way to reliably create the sysfs entry.
> 
> Ah, right.
> 
> > Should we create "func,n" in klp_init_object_loaded() instead of
> > klp_init_func()?
> 
> So that the function entries in sysfs would be created only when the 
> object is loaded? Well, why not, but in that case it could easily confuse 
> the user.

Maybe, but I think it would be fine if we document it.  It should only
be relied on by tools, anyway.

> Object entry would be empty for not loaded object. I would not 
> dare to propose to remove such object entries. It would make things worse. 

Why would removing an empty object entry make things worse?

> So maybe we could introduce an attribute in sysfs object entry which would 
> say if the object is loaded or not. Or something like that.

Hm, I'm not sure I see how this would help.

> Hm, we can easily mess this up :)

Agreed 100% :-)

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