From mboxrd@z Thu Jan 1 00:00:00 1970 Return-Path: Received: (majordomo@vger.kernel.org) by vger.kernel.org via listexpand id S1755608AbcEQOny (ORCPT ); Tue, 17 May 2016 10:43:54 -0400 Received: from mx1.redhat.com ([209.132.183.28]:39290 "EHLO mx1.redhat.com" rhost-flags-OK-OK-OK-OK) by vger.kernel.org with ESMTP id S1751243AbcEQOnw (ORCPT ); Tue, 17 May 2016 10:43:52 -0400 Date: Tue, 17 May 2016 09:43:49 -0500 From: Josh Poimboeuf To: Ingo Molnar Cc: Matt Fleming , Linus Torvalds , Alex Thorlton , Linux Kernel Mailing List , Thomas Gleixner , "H. Peter Anvin" , Andrew Morton , Peter Zijlstra , Borislav Petkov , Andy Lutomirski , Steven Rostedt , Denys Vlasenko Subject: [PATCH] x86/asm/entry: fix stack return address retrieval in thunk Message-ID: <20160517144349.oldftkpo2prpqh56@treble> References: <20160516144648.GA22999@gmail.com> <20160517090434.GA21993@codeblueprint.co.uk> <20160517094655.GB21993@codeblueprint.co.uk> <20160517102049.GB26924@gmail.com> MIME-Version: 1.0 Content-Type: text/plain; charset=utf-8 Content-Disposition: inline In-Reply-To: <20160517102049.GB26924@gmail.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.38]); Tue, 17 May 2016 14:43:52 +0000 (UTC) Sender: linux-kernel-owner@vger.kernel.org List-ID: X-Mailing-List: linux-kernel@vger.kernel.org On Tue, May 17, 2016 at 12:20:49PM +0200, Ingo Molnar wrote: > > * Matt Fleming wrote: > > > On Tue, 17 May, at 10:04:34AM, Matt Fleming wrote: > > > > > > Now I'm wondering whether other users of FRAME_BEGIN/FRAME_END make > > > this same mistake. Coccinelle might be able to detect it perhaps. > > > > A quick bit of sed turned up the code in arch/x86/entry/entry_64.S, > > which looks to suffer from the same bug, > > That would be arch/x86/entry/thunk_64.S, but yeah, good find! > > > /* rdi: arg1 ... normal C conventions. rax is saved/restored. */ > > .macro THUNK name, func, put_ret_addr_in_rdi=0 > > .globl \name > > .type \name, @function > > \name: > > FRAME_BEGIN > > > > /* this one pushes 9 elems, the next one would be %rIP */ > > pushq %rdi > > pushq %rsi > > pushq %rdx > > pushq %rcx > > pushq %rax > > pushq %r8 > > pushq %r9 > > pushq %r10 > > pushq %r11 > > > > .if \put_ret_addr_in_rdi > > /* 9*8(%rsp) is return addr on stack */ > > movq 9*8(%rsp), %rdi > > .endif > > > > With CONFIG_FRAME_POINTER=y 9*8(%rsp) is actually the value of %rbp on > > entry, not the return address. > > This seems to affect: > > #ifdef CONFIG_TRACE_IRQFLAGS > THUNK trace_hardirqs_on_thunk,trace_hardirqs_on_caller,1 > THUNK trace_hardirqs_off_thunk,trace_hardirqs_off_caller,1 > #endif > > and with TRACE_IRQFLAGS we already in most times force frame pointers, such as > when LOCKDEP is enabled: > > config LOCKDEP > bool > depends on DEBUG_KERNEL && TRACE_IRQFLAGS_SUPPORT && STACKTRACE_SUPPORT && LOCKDEP_SUPPORT > ... > select FRAME_POINTER if !MIPS && !PPC && !ARM_UNWIND && !S390 && !MICROBLAZE && !ARC && !SCORE > > Also, unlike the efi_call() case, this thunk_64.S bug affects the quality of > debug/tracing information, not runtime correctness per se. > > still all that is by accident, not by design - this bug should be fixed too. --- From: Josh Poimboeuf Subject: [PATCH] x86/asm/entry: fix stack return address retrieval in thunk With CONFIG_FRAME_POINTER enabled, the thunk can pass a bad return address value to the called function. '9*8(%rsp)' actually gets the frame pointer, not the return address. The only users of the 'put_ret_addr_in_rdi' option are some functions which trace the enabling and disabling of interrupts, so this bug can result in bad debug/tracing information. Fixes: 058fb73274f9 ("x86/asm/entry: Create stack frames in thunk functions") Reported-by: Matt Fleming Signed-off-by: Josh Poimboeuf --- arch/x86/entry/thunk_64.S | 7 ++++--- 1 file changed, 4 insertions(+), 3 deletions(-) diff --git a/arch/x86/entry/thunk_64.S b/arch/x86/entry/thunk_64.S index 98df1fa..dae7ca0 100644 --- a/arch/x86/entry/thunk_64.S +++ b/arch/x86/entry/thunk_64.S @@ -15,9 +15,10 @@ .globl \name .type \name, @function \name: + /* push 1 register if frame pointers are enabled */ FRAME_BEGIN - /* this one pushes 9 elems, the next one would be %rIP */ + /* push 9 registers */ pushq %rdi pushq %rsi pushq %rdx @@ -29,8 +30,8 @@ pushq %r11 .if \put_ret_addr_in_rdi - /* 9*8(%rsp) is return addr on stack */ - movq 9*8(%rsp), %rdi + /* (9*8)+FRAME_OFFSET(%rsp) is return addr on stack */ + movq (9*8)+FRAME_OFFSET(%rsp), %rdi .endif call \func -- 2.4.11