Re: [PATCH] objtool: Detect assembly code falling through to INT3 padding

2018-05-19 Thread hpa
On May 18, 2018 10:51:36 AM PDT, Alexey Dobriyan  wrote:
>On Fri, May 18, 2018 at 09:18:14AM +0200, Ingo Molnar wrote:
>> The concept of built-in kernel tooling working at the machine code
>level is just 
>> so powerful - we should have added our own KCC compiler 20 years ago.
>
>...for two very serious reasons
>
>* C as a language moves very slowly, last help from the comittee were
>  C99 intializers which are OK, but, say, memory model was explictly
>  rejected. However the project expands and becomes more complex much
>  faster than C working group sets up meetings. Compiler authors help
>with extensions but ultimately can not be relied on (see "inline"
>saga).
>
>  Recently everyone was celebrating new and improved min() and max()
> macros admiring creativity and knowledge of intricate language details
>  (me too, don't get this wrong).
>
>  Now this is how it can be done in a language which is not stupid:
>
>   constexpr int min(int a, int b)
>   {
>   return a < b ? a : b;
>   }
>
>  That's literally all. And you can also do
>
>   template
>   void min(T a, char b) = delete;
>
>   template
>   void min(char a, T b) = delete;
>
>  because "char" is char.
>
>  Having control over compiler things like that can be addded more
>  quickly.
>
>
>* insulating the project from the whims of compiler authors who every
>  once in a while use "undefined behaviour" or other kinds of language
>  lawyering to do strange things.
>
>  Other serious projects do this too. Database people use O_DIRECT
>  to insulate themselves from kernel people for the very same reasons.

Sounds like you are proposing switching to C++ more than anything else.

*Steps aside and grabs popcorn*
-- 
Sent from my Android device with K-9 Mail. Please excuse my brevity.


Re: [PATCH] objtool: Detect assembly code falling through to INT3 padding

2018-05-18 Thread Alexey Dobriyan
On Fri, May 18, 2018 at 09:18:14AM +0200, Ingo Molnar wrote:
> The concept of built-in kernel tooling working at the machine code level is 
> just 
> so powerful - we should have added our own KCC compiler 20 years ago.

...for two very serious reasons

* C as a language moves very slowly, last help from the comittee were
  C99 intializers which are OK, but, say, memory model was explictly
  rejected. However the project expands and becomes more complex much
  faster than C working group sets up meetings. Compiler authors help
  with extensions but ultimately can not be relied on (see "inline" saga).

  Recently everyone was celebrating new and improved min() and max()
  macros admiring creativity and knowledge of intricate language details
  (me too, don't get this wrong).

  Now this is how it can be done in a language which is not stupid:

constexpr int min(int a, int b)
{
return a < b ? a : b;
}

  That's literally all. And you can also do

template
void min(T a, char b) = delete;

template
void min(char a, T b) = delete;

  because "char" is char.

  Having control over compiler things like that can be addded more
  quickly.


* insulating the project from the whims of compiler authors who every
  once in a while use "undefined behaviour" or other kinds of language
  lawyering to do strange things.

  Other serious projects do this too. Database people use O_DIRECT
  to insulate themselves from kernel people for the very same reasons.


Re: [PATCH] objtool: Detect assembly code falling through to INT3 padding

2018-05-18 Thread Borislav Petkov
On Fri, May 18, 2018 at 12:27:15AM -0700, H. Peter Anvin wrote:
> On 05/18/18 00:18, Ingo Molnar wrote:
> > 
> > Ok, this is cool, it addresses the robustness problem that INT3 padding 
> > introduced 
> > very nicely.
> > 
> > The concept of built-in kernel tooling working at the machine code level is 
> > just 
> > so powerful - we should have added our own KCC compiler 20 years ago.
> > 
> 
> How many times has this been suggested? ;)

Goes to show we still really need it. Imagine the simplifications and
improvements we'll be able to do. Oh myyy

/me has a wet dream.

-- 
Regards/Gruss,
Boris.

Good mailing practices for 400: avoid top-posting and trim the reply.


Re: [PATCH] objtool: Detect assembly code falling through to INT3 padding

2018-05-18 Thread Ingo Molnar

* Ingo Molnar  wrote:

> 
> * Josh Poimboeuf  wrote:
> 
> > With the following commit:
> > 
> >   51bad67ffbce ("x86/asm: Pad assembly functions with INT3 instructions")
> > 
> > ... asm function alignments are padded with INT3, so it's no longer safe
> > to fall through to an aligned function.  Make sure we catch any such
> > cases with objtool.
> > 
> > Note this only adds checking for 64-bit, since objtool doesn't support
> > x86-32.
> > 
> > Suggested-by: Thomas Gleixner 
> > Suggested-by: Linus Torvalds 
> > Signed-off-by: Josh Poimboeuf 
> > ---
> >  arch/x86/kernel/head_64.S   |  2 --
> >  tools/objtool/arch.h|  3 ++-
> >  tools/objtool/arch/x86/decode.c |  2 +-
> >  tools/objtool/check.c   | 11 ++-
> >  4 files changed, 13 insertions(+), 5 deletions(-)
> 
> Ok, this is cool, it addresses the robustness problem that INT3 padding 
> introduced 
> very nicely.
> 
> The concept of built-in kernel tooling working at the machine code level is 
> just 
> so powerful - we should have added our own KCC compiler 20 years ago.

Hm, so a problem is that if we change the padding on 32-bit as well we won't 
have 
this detection there, because objtool doesn't work on 32-bit.

Thanks,

Ingo


Re: [PATCH] objtool: Detect assembly code falling through to INT3 padding

2018-05-18 Thread H. Peter Anvin
On 05/18/18 00:18, Ingo Molnar wrote:
> 
> Ok, this is cool, it addresses the robustness problem that INT3 padding 
> introduced 
> very nicely.
> 
> The concept of built-in kernel tooling working at the machine code level is 
> just 
> so powerful - we should have added our own KCC compiler 20 years ago.
> 

How many times has this been suggested? ;)

-hpa



Re: [PATCH] objtool: Detect assembly code falling through to INT3 padding

2018-05-18 Thread Ingo Molnar

* Peter Zijlstra  wrote:

> On Thu, May 17, 2018 at 08:49:34AM -0500, Josh Poimboeuf wrote:
> > With the following commit:
> > 
> >   51bad67ffbce ("x86/asm: Pad assembly functions with INT3 instructions")
> > 
> > ... asm function alignments are padded with INT3, so it's no longer safe
> > to fall through to an aligned function.  Make sure we catch any such
> > cases with objtool.
> > 
> > Note this only adds checking for 64-bit, since objtool doesn't support
> > x86-32.
> > 
> > Suggested-by: Thomas Gleixner 
> > Suggested-by: Linus Torvalds 
> > Signed-off-by: Josh Poimboeuf 
> > ---
> >  arch/x86/kernel/head_64.S   |  2 --
> >  tools/objtool/arch.h|  3 ++-
> >  tools/objtool/arch/x86/decode.c |  2 +-
> >  tools/objtool/check.c   | 11 ++-
> >  4 files changed, 13 insertions(+), 5 deletions(-)
> > 
> > diff --git a/arch/x86/kernel/head_64.S b/arch/x86/kernel/head_64.S
> > index 8344dd2f310a..3ed8cec6e765 100644
> > --- a/arch/x86/kernel/head_64.S
> > +++ b/arch/x86/kernel/head_64.S
> > @@ -285,11 +285,9 @@ ENTRY(early_idt_handler_array)
> > .endif
> > pushq $i# 72(%rsp) Vector number
> > jmp early_idt_handler_common
> > -   UNWIND_HINT_IRET_REGS
> > i = i + 1
> > .fill early_idt_handler_array + i*EARLY_IDT_HANDLER_SIZE - ., 1, 0xcc
> > .endr
> > -   UNWIND_HINT_IRET_REGS offset=16
> >  END(early_idt_handler_array)
> >  
> >  early_idt_handler_common:
> 
> As noted on IRC; I got slightly confused what this was about.
> 
> Other than that:
> 
> Acked-by: Peter Zijlstra (Intel) 

And after talking to you on IRC I added this paragraph to the changelog:

  Also remove incorrect and unnecessary unwinder hints from head_64.S
  which caused false positives in the new detection code.

Thanks,

Ingo


Re: [PATCH] objtool: Detect assembly code falling through to INT3 padding

2018-05-18 Thread Ingo Molnar

* Josh Poimboeuf  wrote:

> With the following commit:
> 
>   51bad67ffbce ("x86/asm: Pad assembly functions with INT3 instructions")
> 
> ... asm function alignments are padded with INT3, so it's no longer safe
> to fall through to an aligned function.  Make sure we catch any such
> cases with objtool.
> 
> Note this only adds checking for 64-bit, since objtool doesn't support
> x86-32.
> 
> Suggested-by: Thomas Gleixner 
> Suggested-by: Linus Torvalds 
> Signed-off-by: Josh Poimboeuf 
> ---
>  arch/x86/kernel/head_64.S   |  2 --
>  tools/objtool/arch.h|  3 ++-
>  tools/objtool/arch/x86/decode.c |  2 +-
>  tools/objtool/check.c   | 11 ++-
>  4 files changed, 13 insertions(+), 5 deletions(-)

Ok, this is cool, it addresses the robustness problem that INT3 padding 
introduced 
very nicely.

The concept of built-in kernel tooling working at the machine code level is 
just 
so powerful - we should have added our own KCC compiler 20 years ago.

Thanks,

Ingo


Re: [PATCH] objtool: Detect assembly code falling through to INT3 padding

2018-05-17 Thread Peter Zijlstra
On Thu, May 17, 2018 at 08:49:34AM -0500, Josh Poimboeuf wrote:
> With the following commit:
> 
>   51bad67ffbce ("x86/asm: Pad assembly functions with INT3 instructions")
> 
> ... asm function alignments are padded with INT3, so it's no longer safe
> to fall through to an aligned function.  Make sure we catch any such
> cases with objtool.
> 
> Note this only adds checking for 64-bit, since objtool doesn't support
> x86-32.
> 
> Suggested-by: Thomas Gleixner 
> Suggested-by: Linus Torvalds 
> Signed-off-by: Josh Poimboeuf 
> ---
>  arch/x86/kernel/head_64.S   |  2 --
>  tools/objtool/arch.h|  3 ++-
>  tools/objtool/arch/x86/decode.c |  2 +-
>  tools/objtool/check.c   | 11 ++-
>  4 files changed, 13 insertions(+), 5 deletions(-)
> 
> diff --git a/arch/x86/kernel/head_64.S b/arch/x86/kernel/head_64.S
> index 8344dd2f310a..3ed8cec6e765 100644
> --- a/arch/x86/kernel/head_64.S
> +++ b/arch/x86/kernel/head_64.S
> @@ -285,11 +285,9 @@ ENTRY(early_idt_handler_array)
>   .endif
>   pushq $i# 72(%rsp) Vector number
>   jmp early_idt_handler_common
> - UNWIND_HINT_IRET_REGS
>   i = i + 1
>   .fill early_idt_handler_array + i*EARLY_IDT_HANDLER_SIZE - ., 1, 0xcc
>   .endr
> - UNWIND_HINT_IRET_REGS offset=16
>  END(early_idt_handler_array)
>  
>  early_idt_handler_common:

As noted on IRC; I got slightly confused what this was about.

Other than that:

Acked-by: Peter Zijlstra (Intel)