linux-kernel - Re: [PATCH] livepatch: fix race between enabled_store() and klp_unregister

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:	Mon, 30 Nov 2015 14:53:23 +0100
From:	Petr Mladek <pmladek@...e.com>
To:	Li Bin <huawei.libin@...wei.com>
Cc:	jpoimboe@...hat.com, sjenning@...hat.com, jikos@...nel.org,
	vojtech@...e.com, live-patching@...r.kernel.org,
	linux-kernel@...r.kernel.org, guohanjun@...wei.com,
	dingtianhong@...wei.com, xiexiuqi@...wei.com,
	zhouchengming1@...wei.com
Subject: Re: [PATCH] livepatch: fix race between enabled_store() and
 klp_unregister_patch()

On Mon 2015-11-30 11:54:37, Li Bin wrote:
> There is a potential race as following:
> 
> CPU0                         |  CPU1
> -----------------------------|-----------------------------------
> enabled_store()              |  klp_unregister_patch()
>                              |  |-mutex_lock(&klp_mutex);
> |-mutex_lock(&klp_mutex);    |  |-klp_free_patch();
>                              |  |-mutex_unlock(&klp_mutex);
> |-[process the patch's state]|
> |-mutex_unlock(&klp_mutex)   |
> 
> Fix this race condition by adding klp_is_patch_registered() check in
> enabled_store() after get the lock klp_mutex.

It seems that you are right but the situation is more complicated.
See below.

> 
> Signed-off-by: Li Bin <huawei.libin@...wei.com>
> ---
>  kernel/livepatch/core.c |    5 +++++
>  1 files changed, 5 insertions(+), 0 deletions(-)
> 
> diff --git a/kernel/livepatch/core.c b/kernel/livepatch/core.c
> index db545cb..50af971 100644
> --- a/kernel/livepatch/core.c
> +++ b/kernel/livepatch/core.c
> @@ -614,6 +614,11 @@ static ssize_t enabled_store(struct kobject *kobj, struct kobj_attribute *attr,
>  
>  	mutex_lock(&klp_mutex);
>  
> +	if (!klp_is_patch_registered(patch)) {

How is it guaranteed that "patch" is still a valid pointer, please?

I think that we also need to modify klp_unregister_patch().
It must not finish until the patch->kobj is destroyed.
Otherwise, the patch module would get removed and
the "patch" would be invalid.

I guess that enabled_store() takes reference of the patch->kobj.
Then kobject_put(&patch->kobj) in klp_free_patch() decrements
the refcount but it does not free the object. It means that
it does not wait for  enabled_store() to finish. I am not 100%
sure because the kobject/sysfs code is quite complex.

Anyway, we should refuse to call klp_unregister_patch() at all
in the current implementation. It is not safe because we
do not know if the patch is still used or not.

Note that if you disable the patch, you are only sure that
the new function will not longer be called. But you
do _not_ know if all the existing calls have already finished.
A process might sleep inside the function from the patch.
I guess that it will be possible after we introduce
a more complex consistency algorithm for switching
the patches.

Best Regards,
Petr

> +		ret = -EINVAL;
> +		goto err;
> +	}
> +
>  	if (val == patch->state) {
>  		/* already in requested state */
>  		ret = -EINVAL;
> -- 
> 1.7.1
> 
> --
> To unsubscribe from this list: send the line "unsubscribe live-patching" in
> the body of a message to majordomo@...r.kernel.org
> More majordomo info at  http://vger.kernel.org/majordomo-info.html
--
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/