linux-kernel - Re: [PATCH v2] livepatch: allow removal of a disabled patch

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]
Message-ID: <20160607093620.GC10857@pathway.suse.cz>
Date:	Tue, 7 Jun 2016 11:36:20 +0200
From:	Petr Mladek <pmladek@...e.com>
To:	Miroslav Benes <mbenes@...e.cz>
Cc:	jpoimboe@...hat.com, jeyu@...hat.com, jikos@...nel.org,
	jslaby@...e.cz, live-patching@...r.kernel.org,
	linux-kernel@...r.kernel.org, huawei.libin@...wei.com,
	minfei.huang@...oo.com
Subject: Re: [PATCH v2] livepatch: allow removal of a disabled patch

On Wed 2016-06-01 10:31:59, Miroslav Benes wrote:
> Currently we do not allow patch module to unload since there is no
> method to determine if a task is still running in the patched code.
> 
> The consistency model gives us the way because when the unpatching
> finishes we know that all tasks were marked as safe to call an original
> function. Thus every new call to the function calls the original code
> and at the same time no task can be somewhere in the patched code,
> because it had to leave that code to be marked as safe.
> 
> We can safely let the patch module go after that.
> 
> Completion is used for synchronization between module removal and sysfs
> infrastructure in a similar way to commit 942e443127e9 ("module: Fix
> mod->mkobj.kobj potentially freed too early").
> 
> Note that we still do not allow the removal for immediate model, that is
> no consistency model. The module refcount may increase in this case if
> somebody disables and enables the patch several times. This should not
> cause any harm.
> 
> With this change a call to try_module_get() is moved to
> __klp_enable_patch from klp_register_patch to make module reference
> counting symmetric (module_put() is in a patch disable path) and to
> allow to take a new reference to a disabled module when being enabled.
> 
> Also all kobject_put(&patch->kobj) calls are moved outside of klp_mutex
> lock protection to prevent a deadlock situation when
> klp_unregister_patch is called and sysfs directories are removed. There
> is no need to do the same for other kobject_put() callsites as we
> currently do not have their sysfs counterparts.
> 
> Signed-off-by: Miroslav Benes <mbenes@...e.cz>
> ---
> Based on Josh's v2 of the consistency model.
> 
> diff --git a/kernel/livepatch/core.c b/kernel/livepatch/core.c
> index d55701222b49..a649186fb4af 100644
> --- a/kernel/livepatch/core.c
> +++ b/kernel/livepatch/core.c
> @@ -459,6 +472,15 @@ static ssize_t enabled_store(struct kobject *kobj, struct kobj_attribute *attr,
>  
>  	mutex_lock(&klp_mutex);
>  
> +	if (!klp_is_patch_registered(patch)) {
> +		/*
> +		 * Module with the patch could either disappear meanwhile or is
> +		 * not properly initialized yet.
> +		 */
> +		ret = -EINVAL;
> +		goto err;
> +	}
> +
>  	if (patch->enabled == val) {
>  		/* already in requested state */
>  		ret = -EINVAL;
> @@ -700,11 +721,14 @@ static int klp_init_patch(struct klp_patch *patch)
>  	mutex_lock(&klp_mutex);
>  
>  	patch->enabled = false;
> +	init_completion(&patch->finish);
>  
>  	ret = kobject_init_and_add(&patch->kobj, &klp_ktype_patch,
>  				   klp_root_kobj, "%s", patch->mod->name);
> -	if (ret)
> -		goto unlock;
> +	if (ret) {
> +		mutex_unlock(&klp_mutex);
> +		return ret;
> +	}
>  
>  	klp_for_each_object(patch, obj) {
>  		ret = klp_init_object(patch, obj);
> @@ -720,9 +744,12 @@ static int klp_init_patch(struct klp_patch *patch)
>  
>  free:
>  	klp_free_objects_limited(patch, obj);
> -	kobject_put(&patch->kobj);
> -unlock:
> +
>  	mutex_unlock(&klp_mutex);
> +

Just for record. The sysfs entry exists but patch->list is not
initialized in this error path. Therefore we could write into

   /sys/.../livepatch_foo/enable

and call enabled_store(). It is safe because enabled_store() calls
klp_is_patch_registered() and it does not need patch->list for this
check. Kudos for the klp_is_patch_registered() implementation.

I write this because it is not obvious and it took me some time
to verify that we are on the safe side.

Well, I would feel more comfortable if we initialize patch->list
above.


> +	kobject_put(&patch->kobj);
> +	wait_for_completion(&patch->finish);
> +
>  	return ret;
>  }
>  
> diff --git a/samples/livepatch/livepatch-sample.c b/samples/livepatch/livepatch-sample.c
> index e34f871e69b1..a84676aa7c62 100644
> --- a/samples/livepatch/livepatch-sample.c
> +++ b/samples/livepatch/livepatch-sample.c
> @@ -82,7 +82,6 @@ static int livepatch_init(void)
>  
>  static void livepatch_exit(void)
>  {
> -	WARN_ON(klp_disable_patch(&patch));
>  	WARN_ON(klp_unregister_patch(&patch));

Just for record. I was curious if the following race:

CPU0				CPU1

rmmod livepatch_foo

  livepatch_exit()

				echo 1> /sys/.../livepatch_foo/enable

				  __klp_enable_patch()

				    lock(&klp_mutex)
				    ...
				    patch->enabled = true;
				    ...
				    unlock(&klp_mutex)

     klp_unregister_patch()

       lock(&klp_mutex);

       if (patch->enabled)
	 ret = -EBUSY;

       unlock(&klp_mutex);


Fortunately, this is not possible. livepatch_exit() is called when
the module is in MODULE_STATE_GOING. It means that try_module_get()
will fail and therefore __klp_enable_patch() will fail.


All in all, the patch looks good to me. I am fine with the completion.
In fact, I like it.

Best Regards,
Petr