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

Groups > linux.kernel > #1340389 > unrolled thread

Re: [PATCH 00/33] Compile-time stack metadata 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 00/33] Compile-time stack metadata validation Ingo Molnar <mingo@kernel.org> - 2016-02-23 09:20 +0100
    Re: [PATCH 00/33] Compile-time stack metadata validation Arnaldo Carvalho de Melo <acme@kernel.org> - 2016-02-23 15:30 +0100
      Re: [PATCH 00/33] Compile-time stack metadata validation Josh Poimboeuf <jpoimboe@redhat.com> - 2016-02-23 16:10 +0100
        Re: [PATCH 00/33] Compile-time stack metadata validation Arnaldo Carvalho de Melo <acme@kernel.org> - 2016-02-23 16:30 +0100
    Re: [PATCH 00/33] Compile-time stack metadata validation Josh Poimboeuf <jpoimboe@redhat.com> - 2016-02-23 16:10 +0100
      Re: [PATCH 00/33] Compile-time stack metadata validation Ingo Molnar <mingo@kernel.org> - 2016-02-24 08:50 +0100
        Re: [PATCH 00/33] Compile-time stack metadata validation Josh Poimboeuf <jpoimboe@redhat.com> - 2016-02-24 17:40 +0100

#1340389 — Re: [PATCH 00/33] Compile-time stack metadata validation

So I tried out this latest stacktool series and it looks mostly good for an 
upstream merge.

To help this effort move forward I've applied the preparatory/fix patches that are 
part of this series to tip:x86/debug - that's 26 out of 31 patches. (I've 
propagated all the acks that the latest submission got into the changelogs.)

I have 5 (easy to address) observations that need to be addressed before we can 
move on with the rest of the merge:

1)

Due to recent changes to x86 exception handling, I get a lot of bogus warnings 
about exception table sizes:

  stacktool: arch/x86/kernel/cpu/mcheck/mce.o: __ex_table size not a multiple of 24
  stacktool: arch/x86/kernel/cpu/mtrr/generic.o: __ex_table size not a multiple of 24
  stacktool: arch/x86/kernel/cpu/mtrr/cleanup.o: __ex_table size not a multiple of 24

this is due to:

  548acf19234d x86/mm: Expand the exception table logic to allow new handling options

2)

The fact that 'stacktool' already checks about assembly details like __ex_table[] 
shows that my review feedback early iterations of this series, that the 
'stacktool' name is too specific, was correct.

We really need to rename it before it gets upstream and the situation gets worse. 
__ex_table[] has obviously nothing to do with the 'stack layout' of binaries.

Another suitable name would be 'asmtool' or 'objtool'. For example the following 
would naturally express what it does:

  objtool check kernel/built-in.o

the name expresses that the tool checks object files, independently of the 
existing toolchain. Its primary purpose right now is the checking of stack layout 
details, but the tool is not limited to that at all.

3)

There's quite a bit of overhead when running the tool on larger object files, most 
prominently in cmd_check():

    62.06%  stacktool        stacktool                              [.] cmd_check
     6.72%  stacktool        stacktool                              [.] find_rela_by_dest_range

I added -g to the CFLAGS, which made it visible to annotated output of perf top:

    0.00 :        40430d:       lea    0x4(%rdx,%rax,1),%r13
         :      find_instruction():
         :                                                  struct section *sec,
         :                                                  unsigned long offset)
         :      {
         :              struct instruction *insn;
         :
         :              list_for_each_entry(insn, &file->insns, list)
    0.03 :        404312:       mov    0x38(%rsp),%rax
    0.00 :        404317:       cmp    %rbp,%rax
    0.00 :        40431a:       jne    404334 <cmd_check+0x5b4>
    0.00 :        40431c:       jmpq   4045ba <cmd_check+0x83a>
    0.00 :        404321:       nopl   0x0(%rax)
    6.14 :        404328:       mov    (%rax),%rax
    0.00 :        40432b:       cmp    %rbp,%rax
    2.02 :        40432e:       je     4045ba <cmd_check+0x83a>
         :                      if (insn->sec == sec && insn->offset == offset)
    0.55 :        404334:       cmp    %r12,0x10(%rax)
   87.91 :        404338:       jne    404328 <cmd_check+0x5a8>
    0.00 :        40433a:       cmp    %r13,0x18(%rax)
    3.36 :        40433e:       jne    404328 <cmd_check+0x5a8>
         :      get_jump_destinations():
         :                               * later in validate_functions().
         :                               */
         :                              continue;
         :                      }
         :
         :                      insn->jump_dest = find_instruction(file, dest_sec, dest_off);
    0.00 :        404340:       mov    %rax,0x48(%rbx)
    0.00 :        404344:       jmpq   4042b0 <cmd_check+0x530>
    0.00 :        404349:       nopl   0x0(%rax)
         :      fprintf():
         :
         :      # ifdef __va_arg_pack
         :      __fortify_function int
         :      fprintf (FILE *__restrict __stream, const char *__restrict __fmt, ...)
         :      {
         :        return __fprintf_chk (__stream, __USE_FORTIFY_LEVEL - 1, __fmt,

that looks like a linear list search? That would suck with thousands of entries.

(If this is non-trivial to improve then we can delay this optimization to a later 
patch.)

4)

I think the various 'STACKTOOL' flags in the kernel source are a bit of a misnomer 
- it's not the tool we want to name but the actual property of the code.

So instead of:

  STACKTOOL_IGNORE_FUNC(__bpf_prog_run);

we should do something like:

  STACK_FRAME_NON_STANDARD(__bpf_prog_run);

see how much more expressive it is? It becomes a function attribute independent of 
the tooling that makes use of it.

Similarly, for the highest level 'don't check these directories' makefile flags, 
I'd suggest, instead of using this rather opaque, tool dependent naming:

  STACKTOOL := n

something more specific, like:

  OBJECT_FILES_NON_STANDARD := y

which makes it clearer what's going on: these are special object files that are 
not the typical kernel object files.

Stacktool (or objtool) would be one consumer of this annotation.

I think Boris made similar observations in past reviews.

5)

Likewise, I think the CONFIG_STACK_VALIDATION=y Kconfig flag does not express that 
we do exception table checks as well - and it does not express all the other 
things we may check in object files in the future.

Something like CONFIG_CHECK_OBJECT_FILES=y would express it, and the help text 
would list all the things the tool is able to checks for at the moment.

-------------------

Please send followup iterations of the series against the tip:x86/debug tree (or 
tip:master), to keep the size of the series down.

Thanks,

	Ingo

[toc] | [next] | [standalone]

#1340690

Em Tue, Feb 23, 2016 at 09:14:06AM +0100, Ingo Molnar escreveu:
> The fact that 'stacktool' already checks about assembly details like __ex_table[] 
> shows that my review feedback early iterations of this series, that the 
> 'stacktool' name is too specific, was correct.
> 
> We really need to rename it before it gets upstream and the situation gets worse. 
> __ex_table[] has obviously nothing to do with the 'stack layout' of binaries.
> 
> Another suitable name would be 'asmtool' or 'objtool'. For example the following 
> would naturally express what it does:
> 
>   objtool check kernel/built-in.o
> 
> the name expresses that the tool checks object files, independently of the 
> existing toolchain. Its primary purpose right now is the checking of stack layout 
> details, but the tool is not limited to that at all.

Removing 'tool' from the tool name would be nice too :-) Making it
easily googlable would be good too, lotsa people complain about 'perf'
being too vague, see for a quick laugher:

http://www.brendangregg.com/perf.html

``Searching for just "perf" finds sites on the police, petroleum, weed
control, and a T-shirt. This is not an official perf page, for either
perf_events or the T-shirt.''

The T-shirt: http://www.brendangregg.com/perf_events/omg-so-perf.jpg

Maybe we should ask Linus to come with some other nice, short,
searchable, funny name like 'git'?

'chob' as in 'check object'?

- Arnaldo

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

#1340738

On Tue, Feb 23, 2016 at 11:27:17AM -0300, Arnaldo Carvalho de Melo wrote:
> Em Tue, Feb 23, 2016 at 09:14:06AM +0100, Ingo Molnar escreveu:
> > The fact that 'stacktool' already checks about assembly details like __ex_table[] 
> > shows that my review feedback early iterations of this series, that the 
> > 'stacktool' name is too specific, was correct.
> > 
> > We really need to rename it before it gets upstream and the situation gets worse. 
> > __ex_table[] has obviously nothing to do with the 'stack layout' of binaries.
> > 
> > Another suitable name would be 'asmtool' or 'objtool'. For example the following 
> > would naturally express what it does:
> > 
> >   objtool check kernel/built-in.o
> > 
> > the name expresses that the tool checks object files, independently of the 
> > existing toolchain. Its primary purpose right now is the checking of stack layout 
> > details, but the tool is not limited to that at all.
> 
> Removing 'tool' from the tool name would be nice too :-) Making it
> easily googlable would be good too, lotsa people complain about 'perf'
> being too vague, see for a quick laugher:
> 
> http://www.brendangregg.com/perf.html
> 
> ``Searching for just "perf" finds sites on the police, petroleum, weed
> control, and a T-shirt. This is not an official perf page, for either
> perf_events or the T-shirt.''
> 
> The T-shirt: http://www.brendangregg.com/perf_events/omg-so-perf.jpg

Yeah, 'tool' in the name is kind of silly, but the above type of
situation is why I prefer 'objtool' over 'obj'.

Though I have to admit I like the idea of making a t-shirt for it ;-)

> Maybe we should ask Linus to come with some other nice, short,
> searchable, funny name like 'git'?
> 
> 'chob' as in 'check object'?

I think 'objtool' is searchable enough.  And I also like the fact that
its name at least gives you an idea of what it does (and eventually it
will do more than just "checking").

-- 
Josh

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

#1340777

Em Tue, Feb 23, 2016 at 09:07:55AM -0600, Josh Poimboeuf escreveu:
> I think 'objtool' is searchable enough.  And I also like the fact that

Yeah, agreed, there is even documentation available for it already:

http://docs.bvstools.com/home/objtool

> its name at least gives you an idea of what it does (and eventually it
> will do more than just "checking").

:-)

- ARnaldo

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

#1340746

Hi Ingo,

On Tue, Feb 23, 2016 at 09:14:06AM +0100, Ingo Molnar wrote:
> So I tried out this latest stacktool series and it looks mostly good for an 
> upstream merge.
> 
> To help this effort move forward I've applied the preparatory/fix patches that are 
> part of this series to tip:x86/debug - that's 26 out of 31 patches. (I've 
> propagated all the acks that the latest submission got into the changelogs.)

Thanks very much for your review and for applying the fixes!

A few issues relating to the merge:

- The tip:x86/debug branch fails to build because it depends on
  ec5186557abb ("x86/asm: Add C versions of frame pointer macros") which
  is in tip:x86/asm.

- As Pavel mentioned, the tip-bot seems to be spitting out garbage
  emails from:
  =?UTF-8?B?dGlwLWJvdCBmb3IgSm9zaCBQb2ltYm9ldWYgPHRpcGJvdEB6eXRvci5jb20+?=@zytor.com.

> I have 5 (easy to address) observations that need to be addressed before we can 
> move on with the rest of the merge:
> 
> 1)
> 
> Due to recent changes to x86 exception handling, I get a lot of bogus warnings 
> about exception table sizes:
> 
>   stacktool: arch/x86/kernel/cpu/mcheck/mce.o: __ex_table size not a multiple of 24
>   stacktool: arch/x86/kernel/cpu/mtrr/generic.o: __ex_table size not a multiple of 24
>   stacktool: arch/x86/kernel/cpu/mtrr/cleanup.o: __ex_table size not a multiple of 24
> 
> this is due to:
> 
>   548acf19234d x86/mm: Expand the exception table logic to allow new handling options

Ok, I'll fix it up.

> 2)
> 
> The fact that 'stacktool' already checks about assembly details like __ex_table[] 
> shows that my review feedback early iterations of this series, that the 
> 'stacktool' name is too specific, was correct.
> 
> We really need to rename it before it gets upstream and the situation gets worse. 
> __ex_table[] has obviously nothing to do with the 'stack layout' of binaries.
> 
> Another suitable name would be 'asmtool' or 'objtool'. For example the following 
> would naturally express what it does:
> 
>   objtool check kernel/built-in.o
> 
> the name expresses that the tool checks object files, independently of the 
> existing toolchain. Its primary purpose right now is the checking of stack layout 
> details, but the tool is not limited to that at all.

Fair enough.  I'll rename it to objtool if there are no objections.

> 3)
> 
> There's quite a bit of overhead when running the tool on larger object files, most 
> prominently in cmd_check():
> 
>     62.06%  stacktool        stacktool                              [.] cmd_check
>      6.72%  stacktool        stacktool                              [.] find_rela_by_dest_range
> 
> I added -g to the CFLAGS, which made it visible to annotated output of perf top:
> 
>     0.00 :        40430d:       lea    0x4(%rdx,%rax,1),%r13
>          :      find_instruction():
>          :                                                  struct section *sec,
>          :                                                  unsigned long offset)
>          :      {
>          :              struct instruction *insn;
>          :
>          :              list_for_each_entry(insn, &file->insns, list)
>     0.03 :        404312:       mov    0x38(%rsp),%rax
>     0.00 :        404317:       cmp    %rbp,%rax
>     0.00 :        40431a:       jne    404334 <cmd_check+0x5b4>
>     0.00 :        40431c:       jmpq   4045ba <cmd_check+0x83a>
>     0.00 :        404321:       nopl   0x0(%rax)
>     6.14 :        404328:       mov    (%rax),%rax
>     0.00 :        40432b:       cmp    %rbp,%rax
>     2.02 :        40432e:       je     4045ba <cmd_check+0x83a>
>          :                      if (insn->sec == sec && insn->offset == offset)
>     0.55 :        404334:       cmp    %r12,0x10(%rax)
>    87.91 :        404338:       jne    404328 <cmd_check+0x5a8>
>     0.00 :        40433a:       cmp    %r13,0x18(%rax)
>     3.36 :        40433e:       jne    404328 <cmd_check+0x5a8>
>          :      get_jump_destinations():
>          :                               * later in validate_functions().
>          :                               */
>          :                              continue;
>          :                      }
>          :
>          :                      insn->jump_dest = find_instruction(file, dest_sec, dest_off);
>     0.00 :        404340:       mov    %rax,0x48(%rbx)
>     0.00 :        404344:       jmpq   4042b0 <cmd_check+0x530>
>     0.00 :        404349:       nopl   0x0(%rax)
>          :      fprintf():
>          :
>          :      # ifdef __va_arg_pack
>          :      __fortify_function int
>          :      fprintf (FILE *__restrict __stream, const char *__restrict __fmt, ...)
>          :      {
>          :        return __fprintf_chk (__stream, __USE_FORTIFY_LEVEL - 1, __fmt,
> 
> that looks like a linear list search? That would suck with thousands of entries.
> 
> (If this is non-trivial to improve then we can delay this optimization to a later 
> patch.)

Yeah, I agree that the linear list search isn't good.  I still need to
think about the data structures a bit, so if it's ok with you, I'll fix
it with a later patch.

> 4)
> 
> I think the various 'STACKTOOL' flags in the kernel source are a bit of a misnomer 
> - it's not the tool we want to name but the actual property of the code.
> 
> So instead of:
> 
>   STACKTOOL_IGNORE_FUNC(__bpf_prog_run);
> 
> we should do something like:
> 
>   STACK_FRAME_NON_STANDARD(__bpf_prog_run);
> 
> see how much more expressive it is? It becomes a function attribute independent of 
> the tooling that makes use of it.

Ok, STACK_FRAME_NON_STANDARD sounds fine to me.

> Similarly, for the highest level 'don't check these directories' makefile flags, 
> I'd suggest, instead of using this rather opaque, tool dependent naming:
> 
>   STACKTOOL := n
> 
> something more specific, like:
> 
>   OBJECT_FILES_NON_STANDARD := y
> 
> which makes it clearer what's going on: these are special object files that are 
> not the typical kernel object files.
> 
> Stacktool (or objtool) would be one consumer of this annotation.
> 
> I think Boris made similar observations in past reviews.

Sounds reasonable.  With this approach we could probably eventually get
rid of KASAN_SANITIZE.

I'll change it to "OBJECT_FILES_NON_STANDARD := y" if there are no
objections.

Note there are also per-object ignores like:

  STACKTOOL_head_$(BITS).o := n

I can similarly change that to something like:

  OBJECT_FILES_NON_STANDARD_head_$(BITS).o := n

> 5)
> 
> Likewise, I think the CONFIG_STACK_VALIDATION=y Kconfig flag does not express that 
> we do exception table checks as well - and it does not express all the other 
> things we may check in object files in the future.
> 
> Something like CONFIG_CHECK_OBJECT_FILES=y would express it, and the help text 
> would list all the things the tool is able to checks for at the moment.

Hm, I'm not really sure about this.  Yes, the tool could potentially do
other types of checks, but is it necessary to lump them all together
into a single config option?  It does have subcommands after all ;-)

The exception table check reported above is very basic and doesn't serve
any useful purpose other than supporting the goal of validating the
stack.

However, I don't feel strong enough about it to hold up the merge any
longer, so I'll plan to make the change unless I hear back from you.

> Please send followup iterations of the series against the tip:x86/debug tree (or 
> tip:master), to keep the size of the series down.

Will do, thanks!

-- 
Josh

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

#1341625

* Josh Poimboeuf <jpoimboe@redhat.com> wrote:

> Hi Ingo,
> 
> On Tue, Feb 23, 2016 at 09:14:06AM +0100, Ingo Molnar wrote:
> > So I tried out this latest stacktool series and it looks mostly good for an 
> > upstream merge.
> > 
> > To help this effort move forward I've applied the preparatory/fix patches that are 
> > part of this series to tip:x86/debug - that's 26 out of 31 patches. (I've 
> > propagated all the acks that the latest submission got into the changelogs.)
> 
> Thanks very much for your review and for applying the fixes!
> 
> A few issues relating to the merge:
> 
> - The tip:x86/debug branch fails to build because it depends on
>   ec5186557abb ("x86/asm: Add C versions of frame pointer macros") which
>   is in tip:x86/asm.

Indeed...

> - As Pavel mentioned, the tip-bot seems to be spitting out garbage
>   emails from:
>   =?UTF-8?B?dGlwLWJvdCBmb3IgSm9zaCBQb2ltYm9ldWYgPHRpcGJvdEB6eXRvci5jb20+?=@zytor.com.

Yeah, hpa fixed that meanwhile.

Due to the above bad base I rebased the tree (to a x86/asm base), so there will be 
a new round of (hopefully readable) tip-bot notifications. I'll push it out after 
a bit of testing.

> > 5)
> > 
> > Likewise, I think the CONFIG_STACK_VALIDATION=y Kconfig flag does not express that 
> > we do exception table checks as well - and it does not express all the other 
> > things we may check in object files in the future.
> > 
> > Something like CONFIG_CHECK_OBJECT_FILES=y would express it, and the help text 
> > would list all the things the tool is able to checks for at the moment.
> 
> Hm, I'm not really sure about this.  Yes, the tool could potentially do
> other types of checks, but is it necessary to lump them all together
> into a single config option?  It does have subcommands after all ;-)

lol ;-)

Ok, I'm fine with CONFIG_STACK_VALIDATION=y as well.

Thanks,

	Ingo

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

#1342195

On Wed, Feb 24, 2016 at 08:40:54AM +0100, Ingo Molnar wrote:
> 
> * Josh Poimboeuf <jpoimboe@redhat.com> wrote:
> 
> > Hi Ingo,
> > 
> > On Tue, Feb 23, 2016 at 09:14:06AM +0100, Ingo Molnar wrote:
> > > So I tried out this latest stacktool series and it looks mostly good for an 
> > > upstream merge.
> > > 
> > > To help this effort move forward I've applied the preparatory/fix patches that are 
> > > part of this series to tip:x86/debug - that's 26 out of 31 patches. (I've 
> > > propagated all the acks that the latest submission got into the changelogs.)
> > 
> > Thanks very much for your review and for applying the fixes!
> > 
> > A few issues relating to the merge:
> > 
> > - The tip:x86/debug branch fails to build because it depends on
> >   ec5186557abb ("x86/asm: Add C versions of frame pointer macros") which
> >   is in tip:x86/asm.
> 
> Indeed...

FYI, v17 will be based on tip:x86/debug.  But note that, when run
against that branch, it'll spit out a lot of these warnings:

  objtool: arch/x86/ia32/sys_ia32.o: __ex_table size not a multiple of 12
  objtool: arch/x86/ia32/ia32_signal.o: __ex_table size not a multiple of 12
  objtool: arch/x86/entry/common.o: __ex_table size not a multiple of 12

Those warnings mean it's expecting the new exception table format which
was added in:

  548acf19234d ("x86/mm: Expand the exception table logic to allow new handling options")

So that commit is needed to avoid the warnings.

Thanks!

-- 
Josh

[toc] | [prev] | [standalone]

Back to top | Article view | linux.kernel

csiph-web