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: <54C0C294.30908@ispras.ru>
Date:	Thu, 22 Jan 2015 12:27:48 +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


22.01.2015 3:40, Rusty Russell пишет:
> Andrey Tsyvarev <tsyvarev@...ras.ru> writes:
>> 21.01.2015 4:40, Rusty Russell пишет:
>>> Andrey Tsyvarev <tsyvarev@...ras.ru> writes:
>>>> 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.
>>> I don't know that we have any __initdata locks; it would be really
>>> weird.
>>>
>>> But change 'static DEFINE_MUTEX(mutex_param);' to 'static __initdata
>>> DEFINE_MUTEX(mutex_param);' to test.
>> Compiler warns about sections mismatch, but the test works.
>>
>> According to lockdep_free_key_range() code, lock class is cleared not
>> only according to
>> its key(which is equal to lock address in the case of static lock) but
>> also according to its name.
> What happens if you later register another lock at that address, since
> the memory is freed?
Do you mean that scenario:

1) mutex1 is placed in module1 .init.data section,
2) after module1 is initialized, .init.data section is freed,
3) same memory is reused for module2 .data section,
4) mutex2 is placed in module2 .data section at the same address, as 
mutex1 was?

It seems, mutex2 will share lock class with mutex1. That is, lockdep 
will confused:

[kernel/locking/lockdep.c]
707                 if (class->key == key) {
708                         /*
709                          * Huh! same key, different name? Did 
someone trample
710                          * on some memory? We're most confused.
711                          */
712                         WARN_ON_ONCE(class->name != lock->name);
713                         return class;

Things will go worse, when

5) module1 is exited, and lock class for mutex1 will be cleared

because mutex2 will cache lock class which actually does not exist.

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