Re: [PATCH v6 2/4] x86/stackvalidate: Compile-time stack validation

[Josh Poimboeuf]

On Tue, Jul 07, 2015 at 03:57:14PM -0700, Andy Lutomirski wrote:
> On Tue, Jul 7, 2015 at 7:54 AM, Josh Poimboeuf <jpoimboe@xxxxxxxxxx> wrote:
> > 5. A callable function may not jump to a dynamically determined address.
> >    Such jumps can't be validated since the jump destination is unknown
> >    at compile time.
> 
> Hmm.  Are there no switch statements that get optimized into jump
> tables for which this breaks?

Yeah.  I discovered this the hard way and had to add support for reading
the switch jump tables.  I should probably make that clearer here in the
changelog and in the documentation.

> > 6. A callable function may not execute a context switching instruction.
> >    The only code which needs to do context switches is kernel entry
> >    code, which shouldn't be annotated to be in callable functions
> >    anyway.
> >
> > It currently only supports x86_64.  I tried to make the code generic so
> > that support for other architectures can hopefully be plugged in
> > relatively easily.
> >
> > Currently with my Fedora config it's reporting over 1400 warnings, but
> > most of them are duplicates.  The warnings affect 37 .c files and 18 .S
> > files.  The C file warnings are generally due to inline assembly, which
> > doesn't seem to play nice with frame pointers.
> 
> This issue might be worth bringing up on the gcc and binutils lists.
> If we need better toolchain support, let's ask for it.

I think we found a good solution for this.  See my update at:

  https://lkml.kernel.org/r/20150707223519.GA31294@xxxxxxxxxxxxxxxxx

> > +2. stackvalidate: asm_file.o: .text+0x53: return instruction outside of a callable function
> > +
> > +   A return instruction was detected, but stackvalidate couldn't find a
> > +   way for a callable function to reach the instruction.
> > +
> > +   If the return instruction is inside (or reachable from) a callable
> > +   function, the function needs to be annotated with the PROC/ENDPROC
> > +   macros.
> > +
> > +   If you _really_ need a return instruction outside of a function, and
> > +   are 100% sure that it won't affect stack traces, you can tell
> > +   stackvalidate to ignore it.  See the "Adding exceptions" section
> > +   below.
> 
> This will happen as soon as someone implements
> iretless-return-to-kernel for real.  We can add an exception :)
> 
> > +
> > +5. stackvalidate: asm_file.o: func()+0x6: context switch from callable function
> 
> This description threw me off for a bit.  When I read "context
> switch", I thought of __switch_to.  Would something like "kernel
> entry/exit instruction" instead of "context switch" be clearer?

Yeah, I'll rename it.

> > +
> > +   This is a context switch instruction like sysenter or sysret.  Such
> > +   instructions aren't allowed in a callable function, and are most
> > +   likely part of kernel entry code.
> > +
> > +   If the instruction isn't actually in a callable function, change
> > +   ENDPROC to END.
> > +
> > +
> > +6. stackvalidate: asm_file.o: func()+0x26: jump to outside file from callable function
> > +   or
> > +   stackvalidate: asm_file.o: func()+0xd9: jump to dynamic address from callable function
> > +
> > +   These are constraints imposed by stackvalidate so that it can
> > +   properly analyze all jump targets.  Dynamic jump targets and jumps to
> > +   code in other object files aren't allowed.
> 
> Does this not trigger due to optimized sibling calls to different files?

This is a great point.  With CONFIG_FRAME_POINTER it's not a problem,
because it adds -fno-optimize-sibling-calls.

Without it, I think stackvalidate would spit out a ton of "jump to
outside file" warnings.

I haven't yet looked at the details of how exactly sibling calls work.
I'd assume they're disabled because they break frame pointers somehow.
Any idea if they'd also break DWARF CFI stack traces?

I probably need to do some digging there.  If sibling calls don't break
CFI stack traces and we end up needing them, stackvalidate might need to
analyze the entire kernel image at once instead of its current per-.o
checking.

Anyway, thanks a bunch for all your insightful feedback Andy!

-- 
Josh
--
To unsubscribe from this list: send the line "unsubscribe linux-kernel" in
the body of a message to majordomo@xxxxxxxxxxxxxxx
More majordomo info at  http://vger.kernel.org/majordomo-info.html
Please read the FAQ at  http://www.tux.org/lkml/