Return-Path: Received: (majordomo@vger.kernel.org) by vger.kernel.org via listexpand id S1752697AbdGMSAO (ORCPT ); Thu, 13 Jul 2017 14:00:14 -0400 Received: from mx1.redhat.com ([209.132.183.28]:51694 "EHLO mx1.redhat.com" rhost-flags-OK-OK-OK-OK) by vger.kernel.org with ESMTP id S1751157AbdGMSAM (ORCPT ); Thu, 13 Jul 2017 14:00:12 -0400 DMARC-Filter: OpenDMARC Filter v1.3.2 mx1.redhat.com 625937F406 Authentication-Results: ext-mx01.extmail.prod.ext.phx2.redhat.com; dmarc=none (p=none dis=none) header.from=redhat.com Authentication-Results: ext-mx01.extmail.prod.ext.phx2.redhat.com; spf=pass smtp.mailfrom=jpoimboe@redhat.com DKIM-Filter: OpenDKIM Filter v2.11.0 mx1.redhat.com 625937F406 Date: Thu, 13 Jul 2017 13:00:01 -0500 From: Josh Poimboeuf To: Matthias Kaehlcke Cc: Chris J Arges , Borislav Petkov , Thomas Gleixner , Ingo Molnar , "H . Peter Anvin" , x86@kernel.org, linux-kernel@vger.kernel.org, Douglas Anderson , Michael Davidson , Greg Hackmann , Nick Desaulniers , Stephen Hines , Kees Cook , Arnd Bergmann , Bernhard.Rosenkranzer@linaro.org Subject: Re: [PATCH] Revert "x86/uaccess: Add stack frame output operand in get_user() inline asm" Message-ID: <20170713180001.mvwzdmudht56hdk5@treble> References: <20170712212744.23660-1-mka@chromium.org> <20170712221242.puv5illztsla4nph@treble> <20170712222040.GD95735@google.com> <20170712223547.fyra43dizqooosbs@treble> <20170712223630.gb237t4vhrqeu5zd@treble> <20170712232213.GE95735@google.com> MIME-Version: 1.0 Content-Type: text/plain; charset=utf-8 Content-Disposition: inline In-Reply-To: <20170712232213.GE95735@google.com> User-Agent: Mutt/1.6.0.1 (2016-04-01) X-Greylist: Sender IP whitelisted, not delayed by milter-greylist-4.5.16 (mx1.redhat.com [10.5.110.25]); Thu, 13 Jul 2017 18:00:12 +0000 (UTC) Sender: linux-kernel-owner@vger.kernel.org List-ID: X-Mailing-List: linux-kernel@vger.kernel.org Content-Length: 6066 Lines: 118 On Wed, Jul 12, 2017 at 04:22:13PM -0700, Matthias Kaehlcke wrote: > El Wed, Jul 12, 2017 at 05:36:30PM -0500 Josh Poimboeuf ha dit: > > > On Wed, Jul 12, 2017 at 05:35:47PM -0500, Josh Poimboeuf wrote: > > > On Wed, Jul 12, 2017 at 03:20:40PM -0700, Matthias Kaehlcke wrote: > > > > > This is admittedly an awkward way of achieving this goal, but it's the > > > > > only way I know how to do it with GCC. > > > > > > > > > > What extra instruction does clang add? > > > > > > > > I was looking at the get_user() call in drm_mode_setcrtc(). The code > > > > generated by clang without the patch is: > > > > > > > > if (get_user(out_id, &set_connectors_ptr[i])) { > > > > ffffffff81386955: 4a 8d 04 bd 00 00 00 lea 0x0(,%r15,4),%rax > > > > ffffffff8138695c: 00 > > > > ffffffff8138695d: 49 03 06 add (%r14),%rax > > > > ffffffff81386960: e8 2b a5 f0 ff callq ffffffff81290e90 <__get_user_4> > > > > > > > > And with the patch: > > > > > > > > if (get_user(out_id, &set_connectors_ptr[i])) { > > > > ffffffff81386a56: 4a 8d 04 bd 00 00 00 lea 0x0(,%r15,4),%rax > > > > ffffffff81386a5d: 00 > > > > ffffffff81386a5e: 49 03 06 add (%r14),%rax > > > > ffffffff81386a61: 48 8b 64 24 28 mov 0x28(%rsp),%rsp > > > > ffffffff81386a66: e8 15 a5 f0 ff callq > > > > ffffffff81290f80 <__get_user_4> > > > > > > Hm, that seems odd. Can you sure the disassembly for the whole > > > function? > > > > Er, share :-) > > Sure, please find below the disassemblies with and without the > patch. The exact extra instruction differs from the one above, the > disassembly above is from a debug session with some 'random' kernel > version (bisect), the ones below from a v4.12ish kernel. At the bottom > you also find a log of a double faults observed with the patch. > > If you are interested in building the kernel with clang yourself I can > provide instructions, it is fairly painless nowadays as long as you > have a recent version of clang (a somewhat older version should also > do for this issue with some extra kernel patches). Here's the reason for the double fault. First it puts zero on the stack at offset -0x58: > ffffffff81367616: 31 c0 xor %eax,%eax > ffffffff81367618: 48 89 45 c8 mov %rax,-0x38(%rbp) > ffffffff8136761c: 45 31 ff xor %r15d,%r15d > ffffffff8136761f: 48 89 45 a8 mov %rax,-0x58(%rbp) Then, later, it copies that zeroed word from the stack to RSP: > ffffffff81367874: 48 8b 65 a8 mov -0x58(%rbp),%rsp Then it double faults because the call instruction tries to write RIP on the stack, but RSP is zero: > ffffffff81367878: e8 73 26 f1 ff callq ffffffff81279ef0 <__get_user_4> Then clang tries to put RSP's value on the stack, at the same stack slot where the original zero was stored (though it never reaches this point): > ffffffff8136787d: 49 89 d4 mov %rdx,%r12 > ffffffff81367880: 48 89 65 a8 mov %rsp,-0x58(%rbp) The panic is consistent with the above. RIP points to the call instruction, RSP is zero: > [ 3.798722] PANIC: double fault, error_code: 0x0 > [ 3.807387] CPU: 1 PID: 605 Comm: frecon Not tainted 4.12.0-00023-g711d82c128ff #107 > [ 3.816040] Hardware name: GOOGLE Squawks, BIOS Google_Squawks.5216.152.76 03/04/2016 > [ 3.824792] task: ffff880075b92f00 task.stack: ffffc90000d6c000 > [ 3.829599] EXT4-fs (mmcblk0p1): re-mounted. Opts: commit=600,data=ordered > [ 3.839092] RIP: 0010:drm_mode_setcrtc+0x328/0x51f > [ 3.844443] RSP: 0018:0000000000000000 EFLAGS: 00010206 > [ 3.850280] RAX: 0000559e707c4d60 RBX: 0000000000000000 RCX: 0000000000000008 > [ 3.858253] RDX: 0000000000000001 RSI: ffffc90000d6fcc8 RDI: ffffffff81367805 > [ 3.866225] RBP: ffffc90000d6fd90 R08: 00000000014000c0 R09: 0000000000000308 > [ 3.874199] R10: 0000000000000300 R11: 0000000000000556 R12: 0000000000000000 > [ 3.882163] R13: ffff880077a25000 R14: ffffc90000d6fdd0 R15: 0000000000000000 > [ 3.890136] FS: 00007fb2dbd62740(0000) GS:ffff88007ad00000(0000) knlGS:0000000000000000 > [ 3.899177] CS: 0010 DS: 0000 ES: 0000 CR0: 0000000080050033 > [ 3.905595] CR2: fffffffffffffff8 CR3: 000000007596b000 CR4: 00000000001006e0 > [ 3.913568] Call Trace: > [ 3.916296] Code: b8 48 8d b5 38 ff ff ff 41 8b 56 08 39 d3 0f 83 85 00 00 00 4c 63 fb 4a 83 24 f8 00 4a 8d 04 bd 00 00 00 00 49 03 06 48 8b 65 a8 73 26 f1 ff 49 89 d4 48 89 65 a8 85 c0 0f 85 a0 00 00 00 ba > [ 3.937440] Kernel panic - not syncing: Machine halted. > [ 3.943268] CPU: 1 PID: 605 Comm: frecon Not tainted 4.12.0-00023-g711d82c128ff #107 > [ 3.951921] Hardware name: GOOGLE Squawks, BIOS Google_Squawks.5216.152.76 03/04/2016 > [ 3.960671] Call Trace: > [ 3.963398] <#DF> > [ 3.965637] __dump_stack+0x19/0x1b > [ 3.969531] dump_stack+0x42/0x60 clang is obviously getting confused by the RSP output constraint. I think it tries to take the constraint literally, since it takes RSP as an output from the inline asm and stores it on the stack. However, that behavior doesn't really make sense for a "register" variable. It also doesn't explain why it's zeroing the register out first. What happens if you try the below patch instead of the revert? Any chance the offending instruction goes away? diff --git a/arch/x86/include/asm/uaccess.h b/arch/x86/include/asm/uaccess.h index 11433f9..beac907 100644 --- a/arch/x86/include/asm/uaccess.h +++ b/arch/x86/include/asm/uaccess.h @@ -171,7 +171,7 @@ __typeof__(__builtin_choose_expr(sizeof(x) > sizeof(0UL), 0ULL, 0UL)) might_fault(); \ asm volatile("call __get_user_%P4" \ : "=a" (__ret_gu), "=r" (__val_gu), "+r" (__sp) \ - : "0" (ptr), "i" (sizeof(*(ptr)))); \ + : "0" (ptr), "i" (sizeof(*(ptr))), "r" (__sp)); \ (x) = (__force __typeof__(*(ptr))) __val_gu; \ __builtin_expect(__ret_gu, 0); \ })