linux-kernel - Re: 0be964be0 "module: Sanitize RCU usage and locking" breaks symbol_put

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: <20150818023137.GI20948@worktop>
Date:	Tue, 18 Aug 2015 04:31:37 +0200
From:	Peter Zijlstra <peterz@...radead.org>
To:	Laura Abbott <labbott@...hat.com>
Cc:	Rusty Russell <rusty@...tcorp.com.au>,
	"Paul E. McKenney" <paulmck@...ux.vnet.ibm.com>,
	Linux Kernel Mailing List <linux-kernel@...r.kernel.org>
Subject: Re: 0be964be0 "module: Sanitize RCU usage and locking" breaks
 symbol_put_addr?

On Mon, Aug 17, 2015 at 04:20:09PM -0700, Laura Abbott wrote:
> Hi,
> 
> We received a few bug backtraces:
> 
> [   41.536933] ------------[ cut here ]------------
> [   41.537545] WARNING: CPU: 1 PID: 813 at kernel/module.c:291 module_assert_mutex_or_preempt+0x49/0x90()
> [   41.538174] Modules linked in: mxl5007t af9013 ... dvb_usb_af9015(+) ... dvb_usb_v2 dvb_core rc_core ...
> [   41.542457] CPU: 1 PID: 813 Comm: systemd-udevd Not tainted 4.2.0-0.rc6.git0.1.fc24.x86_64+debug #1
> ...
> [   41.549994]  [<ffffffff81150529>] module_assert_mutex_or_preempt+0x49/0x90
> [   41.550664]  [<ffffffff81150822>] __module_address+0x32/0x150
> [   41.552684]  [<ffffffff81150956>] __module_text_address+0x16/0x70
> [   41.554701]  [<ffffffff81150f19>] symbol_put_addr+0x29/0x40
> [   41.555392]  [<ffffffffa04b77ad>] dvb_frontend_detach+0x7d/0x90 [dvb_core]

> Based on my understanding, this is spitting a warning that the module_mutex
> isn't held. There's a nice comment in symbol_put_addr right before the call:
> 
>         /* module_text_address is safe here: we're supposed to have reference
>          * to module from symbol_get, so it can't go away. */
>         modaddr = __module_text_address(a);
> 
> so it looks like this is an erroneous warning which shouldn't need the mutex held.
> Any ideas or am I off base here?

That comment is wrong, you still need either preempt disabled or hold
the module mutex because you're going to iterate the data structure in
order to look up the module.

The fact that you hold a reference on the module you're going to
(hopefully) find, doesn't stabilize the data structure.

So I think maybe symbol_put_addr() is broken and it wants something
like:

---
 kernel/module.c | 8 ++++++--
 1 file changed, 6 insertions(+), 2 deletions(-)

diff --git a/kernel/module.c b/kernel/module.c
index b86b7bf1be38..8f051a106676 100644
--- a/kernel/module.c
+++ b/kernel/module.c
@@ -1063,11 +1063,15 @@ void symbol_put_addr(void *addr)
 	if (core_kernel_text(a))
 		return;
 
-	/* module_text_address is safe here: we're supposed to have reference
-	 * to module from symbol_get, so it can't go away. */
+	/*
+	 * Even though we hold a reference on the module; we still need to
+	 * disable preemption in order to safely traverse the data structure.
+	 */
+	preempt_disable();
 	modaddr = __module_text_address(a);
 	BUG_ON(!modaddr);
 	module_put(modaddr);
+	preempt_enable();
 }
 EXPORT_SYMBOL_GPL(symbol_put_addr);
 
--
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/