linux-kernel - Re: next: Crashes in x86 images due to 'locking/rwsem, x86: Clean up ____down

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:	Thu, 12 May 2016 16:46:57 +0200
From:	Borislav Petkov <bp@...e.de>
To:	Guenter Roeck <linux@...ck-us.net>, Ingo Molnar <mingo@...nel.org>,
	Peter Zijlstra <peterz@...radead.org>
Cc:	"linux-next@...r.kernel.org" <linux-next@...r.kernel.org>,
	"linux-kernel@...r.kernel.org" <linux-kernel@...r.kernel.org>
Subject: Re: next: Crashes in x86 images due to 'locking/rwsem, x86: Clean up
 ____down_write()'

On Thu, May 12, 2016 at 03:51:31PM +0200, Borislav Petkov wrote:
> On Thu, May 12, 2016 at 06:34:29AM -0700, Guenter Roeck wrote:
> > Borislav,
> > 
> > your patch 'locking/rwsem, x86: Clean up ____down_write()' causes various
> > crashes in x86 qemu tests.
> 
> Thanks for the report, let me take a look.
> 
> @Ingo: can you please back this one out of the lineup for the merge
> window until I've sorted out the issue?

Ok, I was able to reproduce:

BUG: unable to handle kernel NULL pointer dereference at 00000015
IP: [<c185e094>] down_write+0x24/0x30
*pde = 00000000 
Oops: 0002 [#1] PREEMPT SMP
Modules linked in:
CPU: 0 PID: 1 Comm: swapper/0 Tainted: G S      W       4.6.0-rc7-next-20160511-yocto-standard #1
Hardware name: QEMU Standard PC (i440FX + PIIX, 1996), BIOS 1.7.5-20140531_083030-gandalf 04/01/2014
task: f4d00000 ti: f4d08000 task.ti: f4d08000
EIP: 0060:[<c185e094>] EFLAGS: 00210282 CPU: 0
EIP is at down_write+0x24/0x30
EAX: f4d00000 EBX: f4f6d600 ECX: ffff0001 EDX: 00000001
ESI: 00000168 EDI: c1c2eb68 EBP: f4d09ef4 ESP: f4d09eec
 DS: 007b ES: 007b FS: 00d8 GS: 0000 SS: 0068
CR0: 80050033 CR2: 00000015 CR3: 01ccb000 CR4: 000406d0

We fault here:

c185e070 <down_write>:
c185e070:       55                      push   %ebp
c185e071:       89 e5                   mov    %esp,%ebp
c185e073:       e8 20 2b 00 00          call   c1860b98 <mcount>
c185e078:       b9 01 00 ff ff          mov    $0xffff0001,%ecx
c185e07d:       89 c2                   mov    %eax,%edx
c185e07f:       f0 0f c1 08             lock xadd %ecx,(%eax)
c185e083:       66 85 c9                test   %cx,%cx
c185e086:       74 05                   je     c185e08d <down_write+0x1d>
c185e088:       e8 f7 31 b7 ff          call   c13d1284 <call_rwsem_down_write_failed>
c185e08d:       64 a1 48 59 cb c1       mov    %fs:0xc1cb5948,%eax
c185e093:       5d                      pop    %ebp
c185e094:       89 42 14                mov    %eax,0x14(%edx)		<--- HERE
c185e097:       c3                      ret
c185e098:       90                      nop
c185e099:       8d b4 26 00 00 00 00    lea    0x0(%esi,%eiz,1),%esi

and %edx is 1 (+ 0x14 gives the 00000015 deref addr).

But edx should contain sem. The code does:

        .loc 1 47 0
        movl    %eax, %edx      # sem, sem

	lock;   xadd      %ecx,(%eax)   # tmp91, sem

	call call_rwsem_down_write_failed

	mov    %eax,0x14(%edx)

and if something in that call clobbers %edx, boom! Now I need to think
about how to make gcc reload sem after

	LOCK_CONTENDED(sem, __down_write_trylock, __down_write);

for
	rwsem_set_owner(sem);

Btw, the hunk below seems to fix it. And the comment above those
{save,restore}_common_regs talk about "Save the C-clobbered registers
(%eax, %edx and %ecx)" but the only reg we're stashing is ecx.

Why aren't we stashing edx too?

Ingo, Peter?

---
diff --git a/arch/x86/lib/rwsem.S b/arch/x86/lib/rwsem.S
index a37462a23546..02240807e97a 100644
--- a/arch/x86/lib/rwsem.S
+++ b/arch/x86/lib/rwsem.S
@@ -33,10 +33,12 @@
  * value or just clobbered..
  */
 
-#define save_common_regs \
-       pushl %ecx
+#define save_common_regs       \
+       pushl %ecx;             \
+       pushl %edx
 
-#define restore_common_regs \
+#define restore_common_regs    \
+       popl %edx;              \
        popl %ecx
 
        /* Avoid uglifying the argument copying x86-64 needs to do. */

-- 
Regards/Gruss,
    Boris.

SUSE Linux GmbH, GF: Felix Imendörffer, Jane Smithard, Graham Norton, HRB 21284 (AG Nürnberg)
--