linux-kernel - Re: [PATCH v13 04/12] livepatch: Consolidate klp

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]

Date:   Wed, 21 Nov 2018 15:40:59 +0100
From:   Petr Mladek <pmladek@...e.com>
To:     Miroslav Benes <mbenes@...e.cz>
Cc:     Jiri Kosina <jikos@...nel.org>,
        Josh Poimboeuf <jpoimboe@...hat.com>,
        Jason Baron <jbaron@...mai.com>,
        Joe Lawrence <joe.lawrence@...hat.com>,
        Evgenii Shatokhin <eshatokhin@...tuozzo.com>,
        live-patching@...r.kernel.org, linux-kernel@...r.kernel.org,
        Jessica Yu <jeyu@...nel.org>
Subject: Re: [PATCH v13 04/12] livepatch: Consolidate klp_free functions

On Wed 2018-11-21 14:59:29, Miroslav Benes wrote:
> > -/*
> > - * Free all functions' kobjects in the array up to some limit. When limit is
> > - * NULL, all kobjects are freed.
> > - */
> > -static void klp_free_funcs_limited(struct klp_object *obj,
> > -				   struct klp_func *limit)
> > +static void klp_free_funcs(struct klp_object *obj)
> >  {
> >  	struct klp_func *func;
> >  
> > -	for (func = obj->funcs; func->old_name && func != limit; func++)
> > -		kobject_put(&func->kobj);
> > +	klp_for_each_func(obj, func) {
> > +		/* Might be called from klp_init_patch() error path. */
> > +		if (func->kobj.state_initialized)
> > +			kobject_put(&func->kobj);
> > +	}
> >  }
> 
> I have not noticed till today, but state_initialized is probably not the 
> best idea. kobject_init_and_add() sets it to 1 in kobject_init() part but 
> then _add() is called which could result in error. So we would end up with 
> state_initialized equal to 1 and kobject reference equal to 0. Later call 
> to kobject_put() in klp_free_funcs() (or elsewhere) would not call 
> ->release method, because refcount would be 0 by then.
> 
> I think that all would end up well, but that does not mean we should not 
> fix it.
> 
> We could use state_in_sysfs, but I do not think it guarantees anything. 
> Both are internal states and maybe we should not rely on them.
> 
> So kref_read() and check the reference?

I have discussed it a bit more with Miroslav in person. It seems that
the best solution is to add our own .initialized flag into
the affected structures.

The thing is that kobject should be used for dynamically allocated
structures. We use them for static structures just to get the sysfs
interface. Let's still use the sysfs functionality but let's
handle the freeing on our own (reduce hijacking kobject
free machinery).

Best Regards,
Petr