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

Groups > linux.kernel > #1310044 > unrolled thread

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

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 v15 13/25] x86/reboot: Add ljmp instructions to stacktool  whitelist Borislav Petkov <bp@alien8.de> - 2016-01-15 11:50 +0100
    Re: [PATCH v15 13/25] x86/reboot: Add ljmp instructions to stacktool  whitelist Ingo Molnar <mingo@kernel.org> - 2016-01-15 12:10 +0100
      Re: [PATCH v15 13/25] x86/reboot: Add ljmp instructions to stacktool  whitelist Borislav Petkov <bp@alien8.de> - 2016-01-15 12:20 +0100
        Re: [PATCH v15 13/25] x86/reboot: Add ljmp instructions to stacktool  whitelist Ingo Molnar <mingo@kernel.org> - 2016-01-15 12:20 +0100
      Re: [PATCH v15 13/25] x86/reboot: Add ljmp instructions to stacktool  whitelist Josh Poimboeuf <jpoimboe@redhat.com> - 2016-01-20 06:50 +0100
        Re: [PATCH v15 13/25] x86/reboot: Add ljmp instructions to stacktool  whitelist "H. Peter Anvin" <hpa@zytor.com> - 2016-01-20 07:00 +0100
          Re: [PATCH v15 13/25] x86/reboot: Add ljmp instructions to stacktool  whitelist Josh Poimboeuf <jpoimboe@redhat.com> - 2016-01-20 07:20 +0100
        Re: [PATCH v15 13/25] x86/reboot: Add ljmp instructions to stacktool  whitelist Borislav Petkov <bp@alien8.de> - 2016-01-20 11:50 +0100

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

On Fri, Jan 15, 2016 at 12:06:52AM -0600, Josh Poimboeuf wrote:
> - xen_cpuid() uses some custom xen instructions which start with
>   XEN_EMULATE_PREFIX.  It corresponds to the following x86 instructions:
> 
>     ffffffff8107e572:       0f 0b                   ud2
>     ffffffff8107e574:       78 65                   js ffffffff8107e5db <xen_get_debugreg+0xa>
>     ffffffff8107e576:       6e                      outsb %ds:(%rsi),(%dx)
> 
>   Apparently(?) xen treats the ud2 special when it's followed by "78 65
>   6e".  This is confusing for stacktool because ud2 is normally a dead
>   end, and it thinks the instructions after it will never run.
>   
>   (In theory stacktool could be taught to understand this hack, but
>   that's a bad idea IMO)

Why, because it is not generic enough?

Well, you could add a cmdline option "--kernel" which is supplied when
checking the kernel and such kernel "idiosyncrasies" are handled only
then and there. And since the tool is part of the kernel, changes to
XEN_EMULATE_PREFIX, will have to be updated in stacktool too...

> - The error path in arch/x86/net/bpf_jit.S uses 'leaveq' to do a double
>   return so that it returns from its caller's context.  stacktool
>   doesn't know how to distinguish this from a frame pointer programming
>   bug.  I think the only way to avoid a whitelist marker here would be
>   to rewrite the bpf code to conform with more traditional rbp usage
>   (but I don't know if that would really be a good idea because it would
>   probably result in slower/more code).

Could also be part of the "--kernel"-specific checking and you could
match the containing ELF symbol bpf_error...

> - __bpf_prog_run() uses a jump table:
> 
>     goto *jumptable[insn->code];
> 
>   stacktool doesn't have an x86 emulator, so it doesn't know how to
>   deterministically follow all possible branches for a dynamic jump.
> 
> - schedule() mucks with the frame pointer which is normally not allowed.

I think if we put all those checks that under --kernel, the tool would
remain generic enough.

-- 
Regards/Gruss,
    Boris.

ECO tip #101: Trim your mails when you reply.

[toc] | [next] | [standalone]

#1310049

* Borislav Petkov <bp@alien8.de> wrote:

> On Fri, Jan 15, 2016 at 12:06:52AM -0600, Josh Poimboeuf wrote:
> > - xen_cpuid() uses some custom xen instructions which start with
> >   XEN_EMULATE_PREFIX.  It corresponds to the following x86 instructions:
> > 
> >     ffffffff8107e572:       0f 0b                   ud2
> >     ffffffff8107e574:       78 65                   js ffffffff8107e5db <xen_get_debugreg+0xa>
> >     ffffffff8107e576:       6e                      outsb %ds:(%rsi),(%dx)
> > 
> >   Apparently(?) xen treats the ud2 special when it's followed by "78 65
> >   6e".  This is confusing for stacktool because ud2 is normally a dead
> >   end, and it thinks the instructions after it will never run.
> >   
> >   (In theory stacktool could be taught to understand this hack, but
> >   that's a bad idea IMO)
> 
> Why, because it is not generic enough?
> 
> Well, you could add a cmdline option "--kernel" which is supplied when
> checking the kernel and such kernel "idiosyncrasies" are handled only
> then and there. And since the tool is part of the kernel, changes to
> XEN_EMULATE_PREFIX, will have to be updated in stacktool too...

So I think because we are talking about less than a dozen annotations, these are 
technicalities - and it might in fact be better to have a single line of obvious 
annotation in a function that does something weird (and arguably all of these 
functions do something weird), than having dozens of lines of code on the tooling 
side to avoid that single line on the kernel side.

That has a documentation value as well.

As long as the annotation itself is not stacktool specific, it should serve as 
documentation as well - such as:

  __non_standard_stack_frame

or:

  __non_C_instructions

?

All of the cases Josh listed involve some sort of special case where we do 
something non-standard. (Where 'standard' == 'regular kernel C function'.)

Thanks,

	Ingo

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

#1310056

On Fri, Jan 15, 2016 at 12:00:00PM +0100, Ingo Molnar wrote:
> All of the cases Josh listed involve some sort of special case where we do 
> something non-standard. (Where 'standard' == 'regular kernel C function'.)

My only worry is that next time we have to do something non-standard,
we'll have to annotate it as well. bpf_jit case-in-point.

OTOH, I guess the generic, tool-agnostic annotation could be a step
in the (more-)right direction as we're saying "we're doing something
special here, and we're stating that fact with this here annotation."

-- 
Regards/Gruss,
    Boris.

ECO tip #101: Trim your mails when you reply.

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

#1310058

* Borislav Petkov <bp@alien8.de> wrote:

> On Fri, Jan 15, 2016 at 12:00:00PM +0100, Ingo Molnar wrote:
> > All of the cases Josh listed involve some sort of special case where we do 
> > something non-standard. (Where 'standard' == 'regular kernel C function'.)
> 
> My only worry is that next time we have to do something non-standard,
> we'll have to annotate it as well. bpf_jit case-in-point.

but that's generally OK: it's not a problem if something non-standard is visibly 
non-standard on the source code level as well.

> OTOH, I guess the generic, tool-agnostic annotation could be a step in the 
> (more-)right direction as we're saying "we're doing something special here, and 
> we're stating that fact with this here annotation."

Yeah, I think so too!

Thanks,

	Ingo

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

#1312890

On Fri, Jan 15, 2016 at 12:00:00PM +0100, Ingo Molnar wrote:
> 
> * Borislav Petkov <bp@alien8.de> wrote:
> 
> > On Fri, Jan 15, 2016 at 12:06:52AM -0600, Josh Poimboeuf wrote:
> > > - xen_cpuid() uses some custom xen instructions which start with
> > >   XEN_EMULATE_PREFIX.  It corresponds to the following x86 instructions:
> > > 
> > >     ffffffff8107e572:       0f 0b                   ud2
> > >     ffffffff8107e574:       78 65                   js ffffffff8107e5db <xen_get_debugreg+0xa>
> > >     ffffffff8107e576:       6e                      outsb %ds:(%rsi),(%dx)
> > > 
> > >   Apparently(?) xen treats the ud2 special when it's followed by "78 65
> > >   6e".  This is confusing for stacktool because ud2 is normally a dead
> > >   end, and it thinks the instructions after it will never run.
> > >   
> > >   (In theory stacktool could be taught to understand this hack, but
> > >   that's a bad idea IMO)
> > 
> > Why, because it is not generic enough?
> > 
> > Well, you could add a cmdline option "--kernel" which is supplied when
> > checking the kernel and such kernel "idiosyncrasies" are handled only
> > then and there. And since the tool is part of the kernel, changes to
> > XEN_EMULATE_PREFIX, will have to be updated in stacktool too...
> 
> So I think because we are talking about less than a dozen annotations, these are 
> technicalities - and it might in fact be better to have a single line of obvious 
> annotation in a function that does something weird (and arguably all of these 
> functions do something weird), than having dozens of lines of code on the tooling 
> side to avoid that single line on the kernel side.
> 
> That has a documentation value as well.
> 
> As long as the annotation itself is not stacktool specific, it should serve as 
> documentation as well - such as:
> 
>   __non_standard_stack_frame
> 
> or:
> 
>   __non_C_instructions
> 
> ?
> 
> All of the cases Josh listed involve some sort of special case where we do 
> something non-standard. (Where 'standard' == 'regular kernel C function'.)

I've now gotten the number of warnings down to 0 (except for a few
staging drivers), even with allyesconfig (with !CONFIG_GCOV).

I've also managed to make stacktool a little smarter such that the
in-code STACKTOOL_IGNORE_INSN markers are no longer needed, woot!

There's still a need for 4 STACKTOOL_IGNORE_FUNC(name) markers in the
entire tree, due to the weird cases I mentioned.  But they're placed
after the functions, so they're much less disruptive.

I'll be posting a v16 soon.

-- 
Josh

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

#1312898

On 01/19/16 21:42, Josh Poimboeuf wrote:
>>>>
>>>>   Apparently(?) xen treats the ud2 special when it's followed by "78 65
>>>>   6e".  This is confusing for stacktool because ud2 is normally a dead
>>>>   end, and it thinks the instructions after it will never run.
>>>>   

Cute.  UD2 followed by three ASCII characters.  I guess that becomes
really fun when using UD2 + a string for error reporting.  Of course, no
software does that...

	-hpa

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

#1312912

On Tue, Jan 19, 2016 at 09:50:31PM -0800, H. Peter Anvin wrote:
> On 01/19/16 21:42, Josh Poimboeuf wrote:
> >>>>
> >>>>   Apparently(?) xen treats the ud2 special when it's followed by "78 65
> >>>>   6e".  This is confusing for stacktool because ud2 is normally a dead
> >>>>   end, and it thinks the instructions after it will never run.
> >>>>   
> 
> Cute.  UD2 followed by three ASCII characters.  I guess that becomes
> really fun when using UD2 + a string for error reporting.  Of course, no
> software does that...

Ah, I completely missed the fact that they're ASCII.  And it spells
"xen" of course :-)

-- 
Josh

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

#1313099

On Tue, Jan 19, 2016 at 11:42:56PM -0600, Josh Poimboeuf wrote:
> I've now gotten the number of warnings down to 0 (except for a few
> staging drivers), even with allyesconfig (with !CONFIG_GCOV).
> 
> I've also managed to make stacktool a little smarter such that the
> in-code STACKTOOL_IGNORE_INSN markers are no longer needed, woot!

Oh wow! :-)

> There's still a need for 4 STACKTOOL_IGNORE_FUNC(name) markers in the
> entire tree, due to the weird cases I mentioned.  But they're placed
> after the functions, so they're much less disruptive.

Sounds nice.

Thanks!

-- 
Regards/Gruss,
    Boris.

ECO tip #101: Trim your mails when you reply.

[toc] | [prev] | [standalone]

Back to top | Article view | linux.kernel

csiph-web