From mboxrd@z Thu Jan 1 00:00:00 1970 From: Petr Mladek Subject: Re: [PATCH v3 21/27] x86/ftrace: Adapt function tracing for PIE support Date: Thu, 24 May 2018 13:40:24 +0200 Message-ID: <20180524114024.pa67zjipy5qcg4tm@pathway.suse.cz> References: <20180523195421.180248-1-thgarnie@google.com> <20180523195421.180248-22-thgarnie@google.com> Mime-Version: 1.0 Content-Type: text/plain; charset="us-ascii" Content-Transfer-Encoding: 7bit Cc: Kate Stewart , Nicolas Pitre , x86@kernel.org, Sergey Senozhatsky , kvm@vger.kernel.org, Len Brown , Peter Zijlstra , Yonghong Song , Christopher Li , Dave Hansen , Dominik Brodowski , linux-kernel@vger.kernel.org, Masahiro Yamada , Jan Beulich , Pavel Machek , "H . Peter Anvin" , kernel-hardening@lists.openwall.com, Christoph Lameter , Alok Kataria , linux-doc@vger.kernel.org, linux-arch@vger.kernel.org, Jonathan Corbet , Herbert Xu , Baoquan He , David Woodhouse , Steven Rostedt Return-path: Content-Disposition: inline In-Reply-To: <20180523195421.180248-22-thgarnie@google.com> List-Unsubscribe: , List-Archive: List-Post: List-Help: List-Subscribe: , Sender: virtualization-bounces@lists.linux-foundation.org Errors-To: virtualization-bounces@lists.linux-foundation.org List-Id: linux-crypto.vger.kernel.org On Wed 2018-05-23 12:54:15, Thomas Garnier wrote: > When using -fPIE/PIC with function tracing, the compiler generates a > call through the GOT (call *__fentry__@GOTPCREL). This instruction > takes 6 bytes instead of 5 on the usual relative call. > > If PIE is enabled, replace the 6th byte of the GOT call by a 1-byte nop > so ftrace can handle the previous 5-bytes as before. > > Position Independent Executable (PIE) support will allow to extended the > KASLR randomization range below the -2G memory limit. > > Signed-off-by: Thomas Garnier > --- > arch/x86/include/asm/ftrace.h | 6 +++-- > arch/x86/include/asm/sections.h | 4 ++++ > arch/x86/kernel/ftrace.c | 42 +++++++++++++++++++++++++++++++-- > 3 files changed, 48 insertions(+), 4 deletions(-) > > diff --git a/arch/x86/include/asm/ftrace.h b/arch/x86/include/asm/ftrace.h > index c18ed65287d5..8f2decce38d8 100644 > --- a/arch/x86/include/asm/ftrace.h > +++ b/arch/x86/include/asm/ftrace.h > @@ -25,9 +25,11 @@ extern void __fentry__(void); > static inline unsigned long ftrace_call_adjust(unsigned long addr) > { > /* > - * addr is the address of the mcount call instruction. > - * recordmcount does the necessary offset calculation. > + * addr is the address of the mcount call instruction. PIE has always a > + * byte added to the start of the function. > */ > + if (IS_ENABLED(CONFIG_X86_PIE)) > + addr -= 1; This seems to modify the address even for modules that are _not_ compiled with PIE, see below. > return addr; > } > > diff --git a/arch/x86/kernel/ftrace.c b/arch/x86/kernel/ftrace.c > index 01ebcb6f263e..73b3c30cb7a3 100644 > --- a/arch/x86/kernel/ftrace.c > +++ b/arch/x86/kernel/ftrace.c > @@ -135,6 +135,44 @@ ftrace_modify_code_direct(unsigned long ip, unsigned const char *old_code, > return 0; > } > > +/* Bytes before call GOT offset */ > +const unsigned char got_call_preinsn[] = { 0xff, 0x15 }; > + > +static int > +ftrace_modify_initial_code(unsigned long ip, unsigned const char *old_code, > + unsigned const char *new_code) > +{ > + unsigned char replaced[MCOUNT_INSN_SIZE + 1]; > + > + ftrace_expected = old_code; > + > + /* > + * If PIE is not enabled or no GOT call was found, default to the > + * original approach to code modification. > + */ > + if (!IS_ENABLED(CONFIG_X86_PIE) || > + probe_kernel_read(replaced, (void *)ip, sizeof(replaced)) || > + memcmp(replaced, got_call_preinsn, sizeof(got_call_preinsn))) > + return ftrace_modify_code_direct(ip, old_code, new_code); And this looks like an attempt to handle modules compiled without PIE. Does it works with the right ip in that case? I wonder if a better solution would be to update scripts/recordmcount.c to store the incremented location into the module. IMPORTANT: I have only vague picture about how this all works. It is possible that I am completely wrong. The code might be correct, especially if you tested this situation. Best Regards, Petr From mboxrd@z Thu Jan 1 00:00:00 1970 From: Petr Mladek Subject: Re: [PATCH v3 21/27] x86/ftrace: Adapt function tracing for PIE support Date: Thu, 24 May 2018 13:40:24 +0200 Message-ID: <20180524114024.pa67zjipy5qcg4tm@pathway.suse.cz> References: <20180523195421.180248-1-thgarnie@google.com> <20180523195421.180248-22-thgarnie@google.com> Mime-Version: 1.0 Content-Type: text/plain; charset="us-ascii" Content-Transfer-Encoding: 7bit Return-path: Content-Disposition: inline In-Reply-To: <20180523195421.180248-22-thgarnie@google.com> List-Unsubscribe: , List-Archive: List-Post: List-Help: List-Subscribe: , Sender: virtualization-bounces@lists.linux-foundation.org Errors-To: virtualization-bounces@lists.linux-foundation.org To: Thomas Garnier , Steven Rostedt Cc: Kate Stewart , Nicolas Pitre , x86@kernel.org, Sergey Senozhatsky , kvm@vger.kernel.org, Len Brown , Peter Zijlstra , Yonghong Song , Christopher Li , Dave Hansen , Dominik Brodowski , linux-kernel@vger.kernel.org, Masahiro Yamada , Jan Beulich , Pavel Machek , "H . Peter Anvin" , kernel-hardening@lists.openwall.com, Christoph Lameter , Alok Kataria , linux-doc@vger.kernel.org, linux-arch@vger.kernel.org, Jonathan Corbet , Herbert Xu , Baoquan He , David Woodhouse List-Id: linux-sparse@vger.kernel.org On Wed 2018-05-23 12:54:15, Thomas Garnier wrote: > When using -fPIE/PIC with function tracing, the compiler generates a > call through the GOT (call *__fentry__@GOTPCREL). This instruction > takes 6 bytes instead of 5 on the usual relative call. > > If PIE is enabled, replace the 6th byte of the GOT call by a 1-byte nop > so ftrace can handle the previous 5-bytes as before. > > Position Independent Executable (PIE) support will allow to extended the > KASLR randomization range below the -2G memory limit. > > Signed-off-by: Thomas Garnier > --- > arch/x86/include/asm/ftrace.h | 6 +++-- > arch/x86/include/asm/sections.h | 4 ++++ > arch/x86/kernel/ftrace.c | 42 +++++++++++++++++++++++++++++++-- > 3 files changed, 48 insertions(+), 4 deletions(-) > > diff --git a/arch/x86/include/asm/ftrace.h b/arch/x86/include/asm/ftrace.h > index c18ed65287d5..8f2decce38d8 100644 > --- a/arch/x86/include/asm/ftrace.h > +++ b/arch/x86/include/asm/ftrace.h > @@ -25,9 +25,11 @@ extern void __fentry__(void); > static inline unsigned long ftrace_call_adjust(unsigned long addr) > { > /* > - * addr is the address of the mcount call instruction. > - * recordmcount does the necessary offset calculation. > + * addr is the address of the mcount call instruction. PIE has always a > + * byte added to the start of the function. > */ > + if (IS_ENABLED(CONFIG_X86_PIE)) > + addr -= 1; This seems to modify the address even for modules that are _not_ compiled with PIE, see below. > return addr; > } > > diff --git a/arch/x86/kernel/ftrace.c b/arch/x86/kernel/ftrace.c > index 01ebcb6f263e..73b3c30cb7a3 100644 > --- a/arch/x86/kernel/ftrace.c > +++ b/arch/x86/kernel/ftrace.c > @@ -135,6 +135,44 @@ ftrace_modify_code_direct(unsigned long ip, unsigned const char *old_code, > return 0; > } > > +/* Bytes before call GOT offset */ > +const unsigned char got_call_preinsn[] = { 0xff, 0x15 }; > + > +static int > +ftrace_modify_initial_code(unsigned long ip, unsigned const char *old_code, > + unsigned const char *new_code) > +{ > + unsigned char replaced[MCOUNT_INSN_SIZE + 1]; > + > + ftrace_expected = old_code; > + > + /* > + * If PIE is not enabled or no GOT call was found, default to the > + * original approach to code modification. > + */ > + if (!IS_ENABLED(CONFIG_X86_PIE) || > + probe_kernel_read(replaced, (void *)ip, sizeof(replaced)) || > + memcmp(replaced, got_call_preinsn, sizeof(got_call_preinsn))) > + return ftrace_modify_code_direct(ip, old_code, new_code); And this looks like an attempt to handle modules compiled without PIE. Does it works with the right ip in that case? I wonder if a better solution would be to update scripts/recordmcount.c to store the incremented location into the module. IMPORTANT: I have only vague picture about how this all works. It is possible that I am completely wrong. The code might be correct, especially if you tested this situation. Best Regards, Petr From mboxrd@z Thu Jan 1 00:00:00 1970 Date: Thu, 24 May 2018 13:40:24 +0200 From: Petr Mladek Subject: Re: [PATCH v3 21/27] x86/ftrace: Adapt function tracing for PIE support Message-ID: <20180524114024.pa67zjipy5qcg4tm@pathway.suse.cz> References: <20180523195421.180248-1-thgarnie@google.com> <20180523195421.180248-22-thgarnie@google.com> MIME-Version: 1.0 Content-Type: text/plain; charset=us-ascii Content-Disposition: inline In-Reply-To: <20180523195421.180248-22-thgarnie@google.com> To: Thomas Garnier , Steven Rostedt Cc: Herbert Xu , "David S . Miller" , Thomas Gleixner , Ingo Molnar , "H . Peter Anvin" , Peter Zijlstra , Josh Poimboeuf , Greg Kroah-Hartman , Philippe Ombredanne , Kate Stewart , Arnaldo Carvalho de Melo , Yonghong Song , Andrey Ryabinin , Kees Cook , Tom Lendacky , "Kirill A . Shutemov" , Andy Lutomirski , Dominik Brodowski , Borislav Petkov , Borislav Petkov , "Rafael J . Wysocki" , Len Brown , Pavel Machek , Juergen Gross , Alok Kataria , Jan Kiszka , Tejun Heo , Christoph Lameter , Dennis Zhou , Boris Ostrovsky , Alexey Dobriyan , Masami Hiramatsu , Cao jin , Francis Deslauriers , "Paul E . McKenney" , Nicolas Pitre , Andrew Morton , Randy Dunlap , "Luis R . Rodriguez" , Arnd Bergmann , Christopher Li , Jason Baron , Mika Westerberg , Lukas Wunner , Dou Liyang , Sergey Senozhatsky , Masahiro Yamada , Ingo Molnar , Nicholas Piggin , "H . J . Lu" , Paolo Bonzini , Radim =?utf-8?B?S3LEjW3DocWZ?= , Joerg Roedel , David Woodhouse , Dave Hansen , Rik van Riel , Jia Zhang , Ricardo Neri , Jonathan Corbet , Jan Beulich , Matthias Kaehlcke , Baoquan He , Jan H =?iso-8859-1?Q?=2E_Sch=F6nherr?= , Daniel Micay , x86@kernel.org, linux-crypto@vger.kernel.org, linux-kernel@vger.kernel.org, linux-pm@vger.kernel.org, virtualization@lists.linux-foundation.org, xen-devel@lists.xenproject.org, linux-arch@vger.kernel.org, linux-sparse@vger.kernel.org, kvm@vger.kernel.org, linux-doc@vger.kernel.org, kernel-hardening@lists.openwall.com List-ID: On Wed 2018-05-23 12:54:15, Thomas Garnier wrote: > When using -fPIE/PIC with function tracing, the compiler generates a > call through the GOT (call *__fentry__@GOTPCREL). This instruction > takes 6 bytes instead of 5 on the usual relative call. > > If PIE is enabled, replace the 6th byte of the GOT call by a 1-byte nop > so ftrace can handle the previous 5-bytes as before. > > Position Independent Executable (PIE) support will allow to extended the > KASLR randomization range below the -2G memory limit. > > Signed-off-by: Thomas Garnier > --- > arch/x86/include/asm/ftrace.h | 6 +++-- > arch/x86/include/asm/sections.h | 4 ++++ > arch/x86/kernel/ftrace.c | 42 +++++++++++++++++++++++++++++++-- > 3 files changed, 48 insertions(+), 4 deletions(-) > > diff --git a/arch/x86/include/asm/ftrace.h b/arch/x86/include/asm/ftrace.h > index c18ed65287d5..8f2decce38d8 100644 > --- a/arch/x86/include/asm/ftrace.h > +++ b/arch/x86/include/asm/ftrace.h > @@ -25,9 +25,11 @@ extern void __fentry__(void); > static inline unsigned long ftrace_call_adjust(unsigned long addr) > { > /* > - * addr is the address of the mcount call instruction. > - * recordmcount does the necessary offset calculation. > + * addr is the address of the mcount call instruction. PIE has always a > + * byte added to the start of the function. > */ > + if (IS_ENABLED(CONFIG_X86_PIE)) > + addr -= 1; This seems to modify the address even for modules that are _not_ compiled with PIE, see below. > return addr; > } > > diff --git a/arch/x86/kernel/ftrace.c b/arch/x86/kernel/ftrace.c > index 01ebcb6f263e..73b3c30cb7a3 100644 > --- a/arch/x86/kernel/ftrace.c > +++ b/arch/x86/kernel/ftrace.c > @@ -135,6 +135,44 @@ ftrace_modify_code_direct(unsigned long ip, unsigned const char *old_code, > return 0; > } > > +/* Bytes before call GOT offset */ > +const unsigned char got_call_preinsn[] = { 0xff, 0x15 }; > + > +static int > +ftrace_modify_initial_code(unsigned long ip, unsigned const char *old_code, > + unsigned const char *new_code) > +{ > + unsigned char replaced[MCOUNT_INSN_SIZE + 1]; > + > + ftrace_expected = old_code; > + > + /* > + * If PIE is not enabled or no GOT call was found, default to the > + * original approach to code modification. > + */ > + if (!IS_ENABLED(CONFIG_X86_PIE) || > + probe_kernel_read(replaced, (void *)ip, sizeof(replaced)) || > + memcmp(replaced, got_call_preinsn, sizeof(got_call_preinsn))) > + return ftrace_modify_code_direct(ip, old_code, new_code); And this looks like an attempt to handle modules compiled without PIE. Does it works with the right ip in that case? I wonder if a better solution would be to update scripts/recordmcount.c to store the incremented location into the module. IMPORTANT: I have only vague picture about how this all works. It is possible that I am completely wrong. The code might be correct, especially if you tested this situation. Best Regards, Petr