[PATCH] init, fix initcall blacklist for modules

[PATCH] init, fix initcall blacklist for modules

[Prarit Bhargava]

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
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@redhat.com>
Cc: Andrew Morton <akpm@linux-foundation.org>
Cc: Thomas Gleixner <tglx@linutronix.de>
Cc: Yang Shi <yang.shi@linaro.org>
Cc: Ingo Molnar <mingo@kernel.org>
Cc: Mel Gorman <mgorman@suse.de>
Cc: Rasmus Villemoes <linux@rasmusvillemoes.dk>
Cc: Kees Cook <keescook@chromium.org>
Cc: Yaowei Bai <baiyaowei@cmss.chinamobile.com>
Cc: Andrey Ryabinin <aryabinin@virtuozzo.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;
 
 	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;
 		}
-- 
1.7.9.3

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

[Rasmus Villemoes]

On Mon, Jun 13 2016, Prarit Bhargava <prarit@redhat.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?

> 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@redhat.com>
> Cc: Andrew Morton <akpm@linux-foundation.org>
> Cc: Thomas Gleixner <tglx@linutronix.de>
> Cc: Yang Shi <yang.shi@linaro.org>
> Cc: Ingo Molnar <mingo@kernel.org>
> Cc: Mel Gorman <mgorman@suse.de>
> Cc: Rasmus Villemoes <linux@rasmusvillemoes.dk>
> Cc: Kees Cook <keescook@chromium.org>
> Cc: Yaowei Bai <baiyaowei@cmss.chinamobile.com>
> Cc: Andrey Ryabinin <aryabinin@virtuozzo.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

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

[Prarit Bhargava]

On 06/13/2016 04:59 PM, Rasmus Villemoes wrote:
> On Mon, Jun 13 2016, Prarit Bhargava <prarit@redhat.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@redhat.com>
>> Cc: Andrew Morton <akpm@linux-foundation.org>
>> Cc: Thomas Gleixner <tglx@linutronix.de>
>> Cc: Yang Shi <yang.shi@linaro.org>
>> Cc: Ingo Molnar <mingo@kernel.org>
>> Cc: Mel Gorman <mgorman@suse.de>
>> Cc: Rasmus Villemoes <linux@rasmusvillemoes.dk>
>> Cc: Kees Cook <keescook@chromium.org>
>> Cc: Yaowei Bai <baiyaowei@cmss.chinamobile.com>
>> Cc: Andrey Ryabinin <aryabinin@virtuozzo.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
> 

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

[Andrew Morton]

On Tue, 14 Jun 2016 05:59:46 -0400 Prarit Bhargava <prarit@redhat.com> wrote:

> On 06/13/2016 04:59 PM, Rasmus Villemoes wrote:
> > On Mon, Jun 13 2016, Prarit Bhargava <prarit@redhat.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.

Well can we please have a v2 with a complete changelog which identifies
when the regression occurred?  That will help in deciding which kernel
versions need fixing.

[PATCH] init, fix initcall blacklist for modules

[Prarit Bhargava]

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
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@redhat.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;
 
 	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;
 		}
-- 
1.7.9.3