linux-kernel - Re: 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: Windows password security audit tool. GUI, reports in PDF.

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

Date:	Tue, 7 Jun 2016 18:39:51 -0400
From:	Jessica Yu <jeyu@...hat.com>
To:	Petr Mladek <pmladek@...e.com>
Cc:	Miroslav Benes <mbenes@...e.cz>, jpoimboe@...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: livepatch: allow removal of a disabled patch

+++ Petr Mladek [07/06/16 11:36 +0200]:
>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.

Hi Petr,

I'm a bit unclear on this, can you clarify what you mean by initialize
patch->list? I don't think any extra initialization is needed here to
be able use a patch->list node with an existing list (klp_patches),
since this field is embedded and the klp_patches list header is
already statically initialized with LIST_HEAD.

Jessica