linux-kernel - Re: [Bug 111541] Race between cat /proc/kallsyms and rmmod

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-next>] [day] [month] [year] [list]

Message-ID: <20160201124925.GT6357@twins.programming.kicks-ass.net>
Date:	Mon, 1 Feb 2016 13:49:25 +0100
From:	Peter Zijlstra <peterz@...radead.org>
To:	Rusty Russell <rusty@...tcorp.com.au>
Cc:	bugzilla-daemon@...zilla.kernel.org,
	Masami Hiramatsu <masami.hiramatsu.pt@...achi.com>,
	linux-kernel@...r.kernel.org
Subject: Re: [Bug 111541] Race between cat /proc/kallsyms and rmmod

Adding lkml to Cc so that there is an actual email record of this.

(I could for example not reply to Masami's later entries).

On Mon, Feb 01, 2016 at 12:02:01PM +1030, Rusty Russell wrote:
> > https://bugzilla.kernel.org/show_bug.cgi?id=111541

> Unfortunately, that would also create a DoS where anyone can stop module
> loading.

> diff --git a/kernel/module.c b/kernel/module.c
> index 8358f4697c0c..9f2de09b6d8c 100644
> --- a/kernel/module.c
> +++ b/kernel/module.c
> @@ -1992,6 +1992,9 @@ static void free_module(struct module *mod)
>  	mod->state = MODULE_STATE_UNFORMED;
>  	mutex_unlock(&module_mutex);
>  
> +	/* kallsyms walks list without mutex; make sure they see UNFORMED */
> +	synchronize_sched();
> +
>  	/* Remove dynamic debug info */
>  	ddebug_remove_module(mod->name);

So it all depends on when this mod->symtab stuff gets freed, and I could
not actually find this.

If this is with module_memfree(), then the above sync_sched() will not
matter.

Nor will those barriers Masami added do anything. The wmb is between a
->state store and a mutex_unlock. The unlock must already ensure the
unlock store happens after all stores inside its critical section.

The rmb doesn't matter because its a dependent load from the mod list.
That is we _must_ first observe the list entry, otherwise we cannot
dereference it to find the mod->state (except of course if you're Alpha,
but in that case the rcu_dereference in list_for_each_entry_rcu() will
have cured this).