From mboxrd@z Thu Jan 1 00:00:00 1970 Return-Path: Received: (majordomo@vger.kernel.org) by vger.kernel.org via listexpand id S965445AbcALRnG (ORCPT ); Tue, 12 Jan 2016 12:43:06 -0500 Received: from mx1.redhat.com ([209.132.183.28]:38046 "EHLO mx1.redhat.com" rhost-flags-OK-OK-OK-OK) by vger.kernel.org with ESMTP id S933917AbcALRnE (ORCPT ); Tue, 12 Jan 2016 12:43:04 -0500 Date: Tue, 12 Jan 2016 11:43:01 -0600 From: Josh Poimboeuf To: Borislav Petkov Cc: Thomas Gleixner , Ingo Molnar , "H. Peter Anvin" , x86@kernel.org, linux-kernel@vger.kernel.org, live-patching@vger.kernel.org, Michal Marek , Peter Zijlstra , Andy Lutomirski , Linus Torvalds , Andi Kleen , Pedro Alves , Namhyung Kim , Bernd Petrovitsch , Chris J Arges , Andrew Morton , Jiri Slaby , Arnaldo Carvalho de Melo Subject: Re: [PATCH v15 13/25] x86/reboot: Add ljmp instructions to stacktool whitelist Message-ID: <20160112174301.GD310@treble.redhat.com> References: <20160112164711.GD22699@pd.tnic> MIME-Version: 1.0 Content-Type: text/plain; charset=utf-8 Content-Disposition: inline In-Reply-To: <20160112164711.GD22699@pd.tnic> User-Agent: Mutt/1.5.23.1-rc1 (2014-03-12) Sender: linux-kernel-owner@vger.kernel.org List-ID: X-Mailing-List: linux-kernel@vger.kernel.org On Tue, Jan 12, 2016 at 05:47:11PM +0100, Borislav Petkov wrote: > On Fri, Dec 18, 2015 at 06:39:27AM -0600, Josh Poimboeuf wrote: > > stacktool reports a false positive warning for the ljmp instruction in > > machine_real_restart(). Normally, ljmp isn't allowed in a function, but > > this is a special case where it's jumping into real mode. > > > > Add the jumps to a whitelist which tells stacktool to ignore them. > > > > Signed-off-by: Josh Poimboeuf > > --- > > arch/x86/kernel/reboot.c | 7 +++++-- > > 1 file changed, 5 insertions(+), 2 deletions(-) > > > > diff --git a/arch/x86/kernel/reboot.c b/arch/x86/kernel/reboot.c > > index 02693dd..1ea1c5e 100644 > > --- a/arch/x86/kernel/reboot.c > > +++ b/arch/x86/kernel/reboot.c > > @@ -9,6 +9,7 @@ > > #include > > #include > > #include > > +#include > > #include > > #include > > #include > > @@ -97,11 +98,13 @@ void __noreturn machine_real_restart(unsigned int type) > > > > /* Jump to the identity-mapped low memory code */ > > #ifdef CONFIG_X86_32 > > - asm volatile("jmpl *%0" : : > > + asm volatile(STACKTOOL_IGNORE_INSN > > + "jmpl *%0;" : : > > "rm" (real_mode_header->machine_real_restart_asm), > > "a" (type)); > > #else > > - asm volatile("ljmpl *%0" : : > > + asm volatile(STACKTOOL_IGNORE_INSN > > + "ljmpl *%0" : : > > "m" (real_mode_header->machine_real_restart_asm), > > "D" (type)); > > #endif > > Well, I can't say that I'm crazy about all those new tools adding > markers to unrelated kernel code. > > Can't you teach stacktool to ignore the whole machine_real_restart() > function simply? Well, these STACKTOOL_IGNORE whitelist markers are only needed in a handful of places, and only for code that does very weird things. Yes, they're a bit ugly, but IMO they also communicate valuable information: "be careful, this code does something very weird." As for whether to put the whitelist info in the code vs hard-coding it in stacktool, I think it's clearer and less "magical" to put them directly in the code. It's also more resilient to future code changes, e.g. if the offending instruction gets moved or if the function gets renamed. And it gives you the ability to more granularly whitelist instructions rather than entire functions, which could cause other offending stack violations in the function to get overlooked. Another thing is that stacktool could be a nice general purpose tool for finding stack issues in other code bases, and so I think requiring it to have hard-coded knowledge about the code base would greatly limit its general usefulness. (Though maybe this problem could be remediated with a user-provided whitelist file which lists functions to be ignored.) -- Josh