Re: [PATCH v15 13/25] x86/reboot: Add ljmp instructions to stacktool whitelist

[Josh Poimboeuf]

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 <jpoimboe@xxxxxxxxxx>
> > ---
> >  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 <linux/sched.h>
> >  #include <linux/tboot.h>
> >  #include <linux/delay.h>
> > +#include <linux/stacktool.h>
> >  #include <acpi/reboot.h>
> >  #include <asm/io.h>
> >  #include <asm/apic.h>
> > @@ -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