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:	Mon, 13 Jun 2016 22:59:07 +0200
From:	Rasmus Villemoes <linux@...musvillemoes.dk>
To:	Prarit Bhargava <prarit@...hat.com>
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 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?

> 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