linux-kernel - Re: [PATCH] kernel/module.c: Free lock-classes if parse

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]

Message-ID: <54BE082A.20207@ispras.ru>
Date:	Tue, 20 Jan 2015 10:47:54 +0300
From:	Andrey Tsyvarev <tsyvarev@...ras.ru>
To:	Rusty Russell <rusty@...tcorp.com.au>
CC:	linux-kernel@...r.kernel.org,
	Peter Zijlstra <peterz@...radead.org>,
	Ingo Molnar <mingo@...hat.com>
Subject: Re: [PATCH] kernel/module.c: Free lock-classes if parse_args failed

20.01.2015 9:37, Rusty Russell пишет:
> Andrey Tsyvarev <tsyvarev@...ras.ru> writes:
>> parse_args call module parameters' .set handlers, which may use locks defined in the module.
>> So, these classes should be freed in case parse_args returns error(e.g. due to incorrect parameter passed).
> Thanks, this seems right.  Applied.
>
> But this makes me ask: where is lockdep_free_key_range() called on the
> module init code?  It doesn't seem to be at all...
As I understand, locks are not allowed to be defined in the module init 
section. So, no needs to call lockdep_free_key_range() for it.
This has a sense: objects from that section are allowed to be used only 
by module->init() function. But a single function call doesn't require 
any synchronization wrt itself.


If it helps, I used module below for verify effect of the patch:

test.c:
---
#include <linux/module.h>
#include <linux/moduleparam.h>

MODULE_AUTHOR("Tsyvarev Andrey");
MODULE_LICENSE("GPL");

static DEFINE_MUTEX(mutex_main);
static DEFINE_MUTEX(mutex_param);

static int param_set(const char* val, const struct kernel_param* kp)
{
     mutex_lock(&mutex_param); //Use mutex defined in the module
     mutex_unlock(&mutex_param);

     return -EINVAL; // Setting this parameter is always invalid
}

static struct kernel_param_ops param_ops =
{
     .set = param_set
};

module_param_cb(param, &param_ops, NULL, S_IRUGO);

static int minit(void)
{
     mutex_lock(&mutex_main); // Use another mutex
     mutex_unlock(&mutex_main);
     return 0;
}

static void mexit(void)
{
}

module_init(minit);
module_exit(mexit);
--

insmod test.ko param=1 # Will fail
insmod test.ko

If kernel is compiled with CONFIG_LOCKDEP, then, without patch, lockdep 
crashes on the second module insertion.

>
> Confused,
> Rusty.
>
>> Signed-off-by: Andrey Tsyvarev <tsyvarev@...ras.ru>
>> ---
>>   kernel/module.c | 3 +++
>>   1 file changed, 3 insertions(+)
>>
>> diff --git a/kernel/module.c b/kernel/module.c
>> index 3965511..2b44de4 100644
>> --- a/kernel/module.c
>> +++ b/kernel/module.c
>> @@ -3311,6 +3311,9 @@ static int load_module(struct load_info *info, const char __user *uargs,
>>   	module_bug_cleanup(mod);
>>   	mutex_unlock(&module_mutex);
>>   
>> +	/* Free lock-classes: */
>> +	lockdep_free_key_range(mod->module_core, mod->core_size);
>> +
>>   	/* we can't deallocate the module until we clear memory protection */
>>   	unset_module_init_ro_nx(mod);
>>   	unset_module_core_ro_nx(mod);
>> -- 
>> 1.8.3.1

-- 
Best regards,

Andrey Tsyvarev
Linux Verification Center, ISPRAS
web:http://linuxtesting.org

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