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:	Tue, 1 Dec 2015 10:46:19 +0800
From:	libin <huawei.libin@...wei.com>
To:	Josh Poimboeuf <jpoimboe@...hat.com>
CC:	<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>,
	Miroslav Benes <mbenes@...e.cz>
Subject: Re: [PATCH] livepatch: fix race between enabled_store() and
 klp_unregister_patch()


Hi Josh,
on 2015/12/1 9:11, Josh Poimboeuf wrote:
> On Mon, Nov 30, 2015 at 11:54:37AM +0800, 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.
> I'm thinking this race isn't possible, and that instead it would
> deadlock.

Good point.  I did not consider the kernfs lock.

> When I try to recreate something similar by putting a delay in
> enabled_store(), klp_free_patch() just sleeps on its call to
> kobject_put() until enabled_store() returns.  The unregister stack looks
> like:
>
>   [<ffffffff812e966b>] __kernfs_remove+0x1fb/0x380
>   [<ffffffff812ea273>] kernfs_remove+0x23/0x40
>   [<ffffffff812ec601>] sysfs_remove_dir+0x51/0x80
>   [<ffffffff81407fb8>] kobject_del+0x18/0x50
>   [<ffffffff8140804a>] kobject_release+0x5a/0x190
>   [<ffffffff81407f27>] kobject_put+0x27/0x50
>   [<ffffffff81128d41>] klp_unregister_patch+0x71/0x80
>
> It seems to be waiting on a lock which is held by the kernfs code which
> called enabled_store().  So when enabled_store() tries to get the
> klp_mutex, it deadlocks.
>
> Miroslav and I previously discussed a few options to fix this:
>
>  https://lkml.kernel.org/r/alpine.LNX.2.00.1502171728520.29490@pobox.suse.cz

Both the options seem good to me. I look forward to the patch. :)
Thanks,
Li Bin



--
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/