linux-kernel - Re: [PATCH v2 1/3] livepatch: Add force sysfs attribute

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 for Android: free password hash cracker in your pocket

[<prev] [next>] [<thread-prev] [thread-next>] [day] [month] [year] [list]

Date:   Mon, 2 Oct 2017 13:05:47 +0200
From:   Petr Mladek <pmladek@...e.com>
To:     Miroslav Benes <mbenes@...e.cz>
Cc:     jpoimboe@...hat.com, jeyu@...nel.org, jikos@...nel.org,
        lpechacek@...e.cz, live-patching@...r.kernel.org,
        linux-kernel@...r.kernel.org
Subject: Re: [PATCH v2 1/3] livepatch: Add force sysfs attribute

On Mon 2017-08-28 16:58:40, Miroslav Benes wrote:
> On Wed, 16 Aug 2017, Petr Mladek wrote:
> 
> > On Thu 2017-08-10 12:48:13, Miroslav Benes wrote:
> > > Add read-write force attribute to livepatch sysfs infrastructure. We can
> > > use it later to force couple of events during a live patching process.
> > > Be it a sending of a fake signal or forcing of the tasks' successful
> > > conversion.
> > > 
> > > It does not make sense to use the force facility when there is no
> > > transaction running (although there is no harm doing that). Therefore we
> > > limit it only to situations when klp_transition_patch variable is set.
> > > Normally, klp_mutex lock should be acquired, because the variable is
> > > shared. However that would hold the action back unnecessarily because of
> > > waiting for the lock, so we omit the lock here. The resulting race
> > > window is harmless (using force when there is no transaction running).
> > > 
> > > diff --git a/kernel/livepatch/core.c b/kernel/livepatch/core.c
> > > index b9628e43c78f..79022b7eca2c 100644
> > > --- a/kernel/livepatch/core.c
> > > +++ b/kernel/livepatch/core.c
> > > @@ -954,6 +990,13 @@ static int __init klp_init(void)
> > >  	if (!klp_root_kobj)
> > >  		return -ENOMEM;
> > >  
> > > +	ret = sysfs_create_group(klp_root_kobj, &klp_sysfs_group);
> > > +	if (ret) {
> > > +		pr_err("cannot create livepatch attributes in sysfs\n");
> > > +		kobject_put(klp_root_kobj);
> > 
> > We need to set klp_root_kobj = NULL here. Or we need to set the global
> > klp_root_kobj only when the attributes are created. Otherwise,
> > klp_initialized() would return true and registering a patch would
> > push the system out of a safe road.
> 
> Oh, right!
>  
> > Note that this actually opens a small race window when the livepatching
> > core pretends to be initialized even when the initialization still
> > might fail. It is rather theoretical but it would be nice to avoid
> > it if it can be done an easy way, e.g. by setting klp_root_kobj later.
> 
> Hm, klp_initialized() uses klp_root_kobj because it was a simple way of 
> detection. Maybe it is time to introduce proper klp_initialized 
> global variable. I'd like it more than setting klp_root_kobj later just 
> for the sake of correctness.
> 
> What do you think?

The global klp_initialized variable sounds fine to me. The current use
of klp_root_kobj is error prone as we see in this patch.

Best Regards,
Petr