Groups | Search | Server Info | Keyboard shortcuts | Login | Register [http] [https] [nntp] [nntps]

Groups > linux.kernel > #1179143 > unrolled thread

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

Back to article view | Back to linux.kernel

This discussion starts older than the indexed window; earlier articles aren't shown. The article labeled Started by below is the oldest one visible, not the original post.

Contents

  Re: [PATCH v6 2/4] x86/stackvalidate: Compile-time stack validation Andy Lutomirski <luto@amacapital.net> - 2015-07-08 01:00 +0200
    Re: [PATCH v6 2/4] x86/stackvalidate: Compile-time stack validation Josh Poimboeuf <jpoimboe@redhat.com> - 2015-07-08 01:30 +0200
      Re: [PATCH v6 2/4] x86/stackvalidate: Compile-time stack validation Andy Lutomirski <luto@amacapital.net> - 2015-07-08 01:40 +0200
        Re: [PATCH v6 2/4] x86/stackvalidate: Compile-time stack validation Josh Poimboeuf <jpoimboe@redhat.com> - 2015-07-08 01:50 +0200

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

On Tue, Jul 7, 2015 at 7:54 AM, Josh Poimboeuf <jpoimboe@redhat.com> 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?

>
> 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.

> +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?

> +
> +   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?


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

[toc] | [next] | [standalone]

#1179156

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@redhat.com> 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@treble.redhat.com

> > +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@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html
Please read the FAQ at  http://www.tux.org/lkml/

[toc] | [prev] | [next] | [standalone]

#1179162

On Tue, Jul 7, 2015 at 4:29 PM, Josh Poimboeuf <jpoimboe@redhat.com> wrote:
> 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@redhat.com> wrote:
>> >
>> > 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@treble.redhat.com

Does that force frame pointer generation?  If so, then once we have a
real kernel unwinder, we might want a non-frame-pointer-forcing
version for better code generation.  (That can wait, of course.)


>> > +
>> > +   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?

They'll certainly prevent unwinding from finding the pre-optimization
caller, but the rest of unwinding should work.  I don't know why we
turn it off, though.

You might want special-case jump-out-of-translation-unit to be okay if
the stack frame is in its initial state.  That is:

func:
     jmp elsewhere

could be considered okay, as could:

func:
     push %rax
     pop %rax
     jmp elsewhere

and similar.

>
> 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!
>

I'm just pretending to be insightful :)

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

[toc] | [prev] | [next] | [standalone]

#1179163

On Tue, Jul 07, 2015 at 04:35:17PM -0700, Andy Lutomirski wrote:
> On Tue, Jul 7, 2015 at 4:29 PM, Josh Poimboeuf <jpoimboe@redhat.com> wrote:
> > 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@redhat.com> wrote:
> >> >
> >> > 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@treble.redhat.com
> 
> Does that force frame pointer generation?  If so, then once we have a
> real kernel unwinder, we might want a non-frame-pointer-forcing
> version for better code generation.  (That can wait, of course.)

I strongly doubt it would force frame pointer generation if
-fomit-frame-pointer is set.  But I'll verify :-)

> >> > +
> >> > +   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?
> 
> They'll certainly prevent unwinding from finding the pre-optimization
> caller, but the rest of unwinding should work.  I don't know why we
> turn it off, though.
> 
> You might want special-case jump-out-of-translation-unit to be okay if
> the stack frame is in its initial state.  That is:
> 
> func:
>      jmp elsewhere
> 
> could be considered okay, as could:
> 
> func:
>      push %rax
>      pop %rax
>      jmp elsewhere
> 
> and similar.

Ah, nice idea.  That might cover all the cases.  I'll try it.

> 
> >
> > 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!
> >
> 
> I'm just pretending to be insightful :)

Insightful or not, your comments have been very helpful!

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

[toc] | [prev] | [standalone]

Back to top | Article view | linux.kernel

csiph-web