linux-kernel - Re: [PATCH] init, fix initcall blacklist for modules

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, 14 Jun 2016 05:59:46 -0400
From:	Prarit Bhargava <prarit@...hat.com>
To:	Rasmus Villemoes <linux@...musvillemoes.dk>
CC:	linux-kernel@...r.kernel.org,
	Andrew Morton <akpm@...ux-foundation.org>,
	Thomas Gleixner <tglx@...utronix.de>,
	Yang Shi <yang.shi@...aro.org>, Ingo Molnar <mingo@...nel.org>,
	Mel Gorman <mgorman@...e.de>,
	Kees Cook <keescook@...omium.org>,
	Yaowei Bai <baiyaowei@...s.chinamobile.com>,
	Andrey Ryabinin <aryabinin@...tuozzo.com>
Subject: Re: [PATCH] init, fix initcall blacklist for modules



On 06/13/2016 04:59 PM, Rasmus Villemoes wrote:
> On Mon, Jun 13 2016, Prarit Bhargava <prarit@...hat.com> wrote:
> 
>> Sorry ... forgot to cc everyone on the last email.
>>
>> P.
>>
>> ----8<----
>>
>> sprint_symbol_no_offset() returns the string "function_name [module_name]"
>> where [module_name] is not printed for built in kernel functions.  This
>> means that the initcall blacklisting code will now always fail when
> 
> I was and am pretty sure that %pf ends up using
> sprint_symbol_no_offset(), so I don't see how this is new. But maybe
> "now" doesn't refer to c8cdd2be21?

Oops.  I can see how you read that that way.  No, this isn't caused by or
"Fixes:" c8cdd2be21.  At some point "%pF" changed its behavior and blacklisting
module_init() functions stopped working.

P.

> 
>> comparing module_init() function names.  This patch resolves the issue by
>> comparing to the length of the function_name.
>>
>> Signed-off-by: Prarit Bhargava <prarit@...hat.com>
>> Cc: Andrew Morton <akpm@...ux-foundation.org>
>> Cc: Thomas Gleixner <tglx@...utronix.de>
>> Cc: Yang Shi <yang.shi@...aro.org>
>> Cc: Ingo Molnar <mingo@...nel.org>
>> Cc: Mel Gorman <mgorman@...e.de>
>> Cc: Rasmus Villemoes <linux@...musvillemoes.dk>
>> Cc: Kees Cook <keescook@...omium.org>
>> Cc: Yaowei Bai <baiyaowei@...s.chinamobile.com>
>> Cc: Andrey Ryabinin <aryabinin@...tuozzo.com>
>> ---
>>  init/main.c |   14 +++++++++++++-
>>  1 file changed, 13 insertions(+), 1 deletion(-)
>>
>> diff --git a/init/main.c b/init/main.c
>> index 4c17fda5c2ff..09a795e91efe 100644
>> --- a/init/main.c
>> +++ b/init/main.c
>> @@ -708,14 +708,26 @@ static bool __init_or_module initcall_blacklisted(initcall_t fn)
>>  {
>>  	struct blacklist_entry *entry;
>>  	char fn_name[KSYM_SYMBOL_LEN];
>> +	char *space;
>> +	int length;
>>  
>>  	if (list_empty(&blacklisted_initcalls))
>>  		return false;
>>  
>>  	sprint_symbol_no_offset(fn_name, (unsigned long)fn);
>> +	/*
>> +	 * fn will be "function_name [module_name]" where [module_name] is not
>> +	 * displayed for built-in initcall functions.  Strip off the
>> +	 * [module_name].
>> +	 */
>> +	space = strchrnul(fn_name, ' ');
>> +	if (!space)
>> +		length = strlen(fn_name);
>> +	else
>> +		length = space - fn_name;
> 
> strchrnul never returns NULL, so this could just be 'length =
> strchrnul(fn_name, ' ') - fn_name;'. But I don't think that's what you
> want anyway: Suppose one has blacklisted "init_foobar", and the function
> pointer resolves to a completely unrelated "init_foo", we'll end up
> falsely also blacklisting that since we're just comparing prefixes.
> 
> May I suggest
> 
>   strreplace(fn_name, ' ', '\0');
> 
> which also seems to match the comment a little better (and eliminates
> the extra variables and the hunk below).
> 
>>  	list_for_each_entry(entry, &blacklisted_initcalls, next) {
>> -		if (!strcmp(fn_name, entry->buf)) {
>> +		if (!strncmp(fn_name, entry->buf, length)) {
>>  			pr_debug("initcall %s blacklisted\n", fn_name);
>>  			return true;
>>  		}
> 
> Rasmus
>