[PATCH 0/4] [GIT PULL][3.12] x86/jump-label: Clean ups and safety checks

[PATCH 0/4] [GIT PULL][3.12] x86/jump-label: Clean ups and safety checks

[Steven Rostedt]

Peter,

These 4 patches were taken from the patchset that did the 2-5 byte
jumps, but had nothing to do with the 2-5 byte jumps itself. They
were clean ups and safety checks that were needed for the jump code.
Although the 2-5 byte jump code is controversial, these cleanups are
not.

The first change is to use the default nops instead of a jmp to next
instruction in the jump label code.

The second is a boot time optimization that doesn't bother changing
the nops if the default nop is the same as the ideal nop.

The third patch adds safety checks like the ftrace function code has.
It makes sure the op code that is being changed is in fact the op code
we expect it to be.

The last patch will output the problem code if it fails the safety
check from patch three.

This is based off of tip/x86/jumplabel, which was equal to v3.11-rc1.

Please pull the latest tip/x86/jumplabel tree, which can be found at:

  git://git.kernel.org/pub/scm/linux/kernel/git/rostedt/linux-trace.git
tip/x86/jumplabel

Head SHA1: fb40d7a8994a3cc7a1e1c1f3258ea8662a366916


Steven Rostedt (4):
      x86/jump-label: Use best default nops for inital jump label calls
      x86/jump-label: Do not bother updating nops if they are correct
      x86/jump-label: Add safety checks to jump label conversions
      x86/jump-label: Show where and what was wrong on errors

----
 arch/x86/include/asm/jump_label.h |    9 +++--
 arch/x86/kernel/jump_label.c      |   70 ++++++++++++++++++++++++++++++++++---
 2 files changed, 73 insertions(+), 6 deletions(-)

[PATCH 1/4] x86/jump-label: Use best default nops for inital jump label calls

[Steven Rostedt]

[-- Attachment #1: 0001-x86-jump-label-Use-best-default-nops-for-inital-jump.patch --]
[-- Type: text/plain, Size: 1551 bytes --]

From: Steven Rostedt <srostedt@redhat.com>

As specified by H. Peter Anvin, the best nops for x86 without knowing
the running computer is:

32bit:
  0x3e, 0x8d, 0x74, 0x26, 0x00 also known as GENERIC_NOP5_ATOMIC

64bit:
  0x0f, 0x1f, 0x44, 0x00, 0x00  also known as P6_NOP5_ATOMIC

Currently the default nop that is used by jump label is:

 0xe9 0x00 0x00 0x00 0x00

Which is really a 5byte jump to the next position.

It's better to use a real nop than a jmp.

Cc: H. Peter Anvin <hpa@linux.intel.com>
Cc: Jason Baron <jbaron@redhat.com>
Signed-off-by: Steven Rostedt <rostedt@goodmis.org>
---
 arch/x86/include/asm/jump_label.h |    9 +++++++--
 1 file changed, 7 insertions(+), 2 deletions(-)

diff --git a/arch/x86/include/asm/jump_label.h b/arch/x86/include/asm/jump_label.h
index 3a16c14..64507f3 100644
--- a/arch/x86/include/asm/jump_label.h
+++ b/arch/x86/include/asm/jump_label.h
@@ -3,18 +3,23 @@
 
 #ifdef __KERNEL__
 
+#include <linux/stringify.h>
 #include <linux/types.h>
 #include <asm/nops.h>
 #include <asm/asm.h>
 
 #define JUMP_LABEL_NOP_SIZE 5
 
-#define STATIC_KEY_INITIAL_NOP ".byte 0xe9 \n\t .long 0\n\t"
+#ifdef CONFIG_X86_64
+# define STATIC_KEY_INIT_NOP P6_NOP5_ATOMIC
+#else
+# define STATIC_KEY_INIT_NOP GENERIC_NOP5_ATOMIC
+#endif
 
 static __always_inline bool arch_static_branch(struct static_key *key)
 {
 	asm goto("1:"
-		STATIC_KEY_INITIAL_NOP
+		".byte " __stringify(STATIC_KEY_INIT_NOP) "\n\t"
 		".pushsection __jump_table,  \"aw\" \n\t"
 		_ASM_ALIGN "\n\t"
 		_ASM_PTR "1b, %l[l_yes], %c0 \n\t"
-- 
1.7.10.4

[PATCH 2/4] x86/jump-label: Do not bother updating nops if they are correct

[Steven Rostedt]

[-- Attachment #1: 0002-x86-jump-label-Do-not-bother-updating-nops-if-they-a.patch --]
[-- Type: text/plain, Size: 1798 bytes --]

From: Steven Rostedt <srostedt@redhat.com>

On boot up, the jump label init function scans all the jump label locations
and converts them to the best nop for the machine. If the nop is already
the ideal nop, do not bother with changing it.

Cc: Jason Baron <jbaron@redhat.com>
Cc: H. Peter Anvin <hpa@zytor.com>
Signed-off-by: Steven Rostedt <rostedt@goodmis.org>
---
 arch/x86/kernel/jump_label.c |   25 ++++++++++++++++++++++++-
 1 file changed, 24 insertions(+), 1 deletion(-)

diff --git a/arch/x86/kernel/jump_label.c b/arch/x86/kernel/jump_label.c
index 2889b3d..ae3d8fb 100644
--- a/arch/x86/kernel/jump_label.c
+++ b/arch/x86/kernel/jump_label.c
@@ -50,10 +50,33 @@ void arch_jump_label_transform(struct jump_entry *entry,
 	put_online_cpus();
 }
 
+static enum {
+	JL_STATE_START,
+	JL_STATE_NO_UPDATE,
+	JL_STATE_UPDATE,
+} jlstate __initdata_or_module = JL_STATE_START;
+
 __init_or_module void arch_jump_label_transform_static(struct jump_entry *entry,
 				      enum jump_label_type type)
 {
-	__jump_label_transform(entry, type, text_poke_early);
+	/*
+	 * This function is called at boot up and when modules are
+	 * first loaded. Check if the default nop, the one that is
+	 * inserted at compile time, is the ideal nop. If it is, then
+	 * we do not need to update the nop, and we can leave it as is.
+	 * If it is not, then we need to update the nop to the ideal nop.
+	 */
+	if (jlstate == JL_STATE_START) {
+		const unsigned char default_nop[] = { STATIC_KEY_INIT_NOP };
+		const unsigned char *ideal_nop = ideal_nops[NOP_ATOMIC5];
+
+		if (memcmp(ideal_nop, default_nop, 5) != 0)
+			jlstate = JL_STATE_UPDATE;
+		else
+			jlstate = JL_STATE_NO_UPDATE;
+	}
+	if (jlstate == JL_STATE_UPDATE)
+		__jump_label_transform(entry, type, text_poke_early);
 }
 
 #endif
-- 
1.7.10.4

[PATCH 3/4] x86/jump-label: Add safety checks to jump label conversions

[Steven Rostedt]

[-- Attachment #1: 0003-x86-jump-label-Add-safety-checks-to-jump-label-conve.patch --]
[-- Type: text/plain, Size: 2681 bytes --]

From: Steven Rostedt <srostedt@redhat.com>

As with all modifying of kernel text, we need to be very paranoid.

When converting the jump label locations to and from nops to jumps
a check has been added to make sure what we are replacing is what we
expect, otherwise we bug.

Cc: H. Peter Anvin <hpa@zytor.com>
Cc: Jason Baron <jbaron@redhat.com>
Signed-off-by: Steven Rostedt <rostedt@goodmis.org>
---
 arch/x86/kernel/jump_label.c |   32 ++++++++++++++++++++++++++++----
 1 file changed, 28 insertions(+), 4 deletions(-)

diff --git a/arch/x86/kernel/jump_label.c b/arch/x86/kernel/jump_label.c
index ae3d8fb..24cf2b2 100644
--- a/arch/x86/kernel/jump_label.c
+++ b/arch/x86/kernel/jump_label.c
@@ -26,16 +26,40 @@ union jump_code_union {
 
 static void __jump_label_transform(struct jump_entry *entry,
 				   enum jump_label_type type,
-				   void *(*poker)(void *, const void *, size_t))
+				   void *(*poker)(void *, const void *, size_t),
+				   int init)
 {
 	union jump_code_union code;
+	const unsigned char *ideal_nop = ideal_nops[NOP_ATOMIC5];
 
 	if (type == JUMP_LABEL_ENABLE) {
+		/*
+		 * We are enabling this jump label. If it is not a nop
+		 * then something must have gone wrong.
+		 */
+		BUG_ON(memcmp((void *)entry->code, ideal_nop, 5) != 0);
+
 		code.jump = 0xe9;
 		code.offset = entry->target -
 				(entry->code + JUMP_LABEL_NOP_SIZE);
-	} else
+	} else {
+		/*
+		 * We are disabling this jump label. If it is not what
+		 * we think it is, then something must have gone wrong.
+		 * If this is the first initialization call, then we
+		 * are converting the default nop to the ideal nop.
+		 */
+		if (init) {
+			const unsigned char default_nop[] = { STATIC_KEY_INIT_NOP };
+			BUG_ON(memcmp((void *)entry->code, default_nop, 5) != 0);
+		} else {
+			code.jump = 0xe9;
+			code.offset = entry->target -
+				(entry->code + JUMP_LABEL_NOP_SIZE);
+			BUG_ON(memcmp((void *)entry->code, &code, 5) != 0);
+		}
 		memcpy(&code, ideal_nops[NOP_ATOMIC5], JUMP_LABEL_NOP_SIZE);
+	}
 
 	(*poker)((void *)entry->code, &code, JUMP_LABEL_NOP_SIZE);
 }
@@ -45,7 +69,7 @@ void arch_jump_label_transform(struct jump_entry *entry,
 {
 	get_online_cpus();
 	mutex_lock(&text_mutex);
-	__jump_label_transform(entry, type, text_poke_smp);
+	__jump_label_transform(entry, type, text_poke_smp, 0);
 	mutex_unlock(&text_mutex);
 	put_online_cpus();
 }
@@ -76,7 +100,7 @@ __init_or_module void arch_jump_label_transform_static(struct jump_entry *entry,
 			jlstate = JL_STATE_NO_UPDATE;
 	}
 	if (jlstate == JL_STATE_UPDATE)
-		__jump_label_transform(entry, type, text_poke_early);
+		__jump_label_transform(entry, type, text_poke_early, 1);
 }
 
 #endif
-- 
1.7.10.4

[PATCH 4/4] x86/jump-label: Show where and what was wrong on errors

[Steven Rostedt]

[-- Attachment #1: 0004-x86-jump-label-Show-where-and-what-was-wrong-on-erro.patch --]
[-- Type: text/plain, Size: 2444 bytes --]

From: Steven Rostedt <srostedt@redhat.com>

When modifying text sections for jump labels, a paranoid check is
performed. If the check fails, the system "bugs". But why it failed
is not shown.

The BUG_ON()s in the jump label update code is replaced with bug_at(ip).
This is a function that will show what pointer failed, and what was
at the location of the failure that made jump label panic.

Signed-off-by: Steven Rostedt <rostedt@goodmis.org>
---
 arch/x86/kernel/jump_label.c |   21 ++++++++++++++++++---
 1 file changed, 18 insertions(+), 3 deletions(-)

diff --git a/arch/x86/kernel/jump_label.c b/arch/x86/kernel/jump_label.c
index 24cf2b2..912a528 100644
--- a/arch/x86/kernel/jump_label.c
+++ b/arch/x86/kernel/jump_label.c
@@ -24,6 +24,18 @@ union jump_code_union {
 	} __attribute__((packed));
 };
 
+static void bug_at(unsigned char *ip, int line)
+{
+	/*
+	 * The location is not an op that we were expecting.
+	 * Something went wrong. Crash the box, as something could be
+	 * corrupting the kernel.
+	 */
+	pr_warning("Unexpected op at %pS [%p] (%02x %02x %02x %02x %02x) %s:%d\n",
+	       ip, ip, ip[0], ip[1], ip[2], ip[3], ip[4], __FILE__, line);
+	BUG();
+}
+
 static void __jump_label_transform(struct jump_entry *entry,
 				   enum jump_label_type type,
 				   void *(*poker)(void *, const void *, size_t),
@@ -37,7 +49,8 @@ static void __jump_label_transform(struct jump_entry *entry,
 		 * We are enabling this jump label. If it is not a nop
 		 * then something must have gone wrong.
 		 */
-		BUG_ON(memcmp((void *)entry->code, ideal_nop, 5) != 0);
+		if (unlikely(memcmp((void *)entry->code, ideal_nop, 5) != 0))
+			bug_at((void *)entry->code, __LINE__);
 
 		code.jump = 0xe9;
 		code.offset = entry->target -
@@ -51,12 +64,14 @@ static void __jump_label_transform(struct jump_entry *entry,
 		 */
 		if (init) {
 			const unsigned char default_nop[] = { STATIC_KEY_INIT_NOP };
-			BUG_ON(memcmp((void *)entry->code, default_nop, 5) != 0);
+			if (unlikely(memcmp((void *)entry->code, default_nop, 5) != 0))
+				bug_at((void *)entry->code, __LINE__);
 		} else {
 			code.jump = 0xe9;
 			code.offset = entry->target -
 				(entry->code + JUMP_LABEL_NOP_SIZE);
-			BUG_ON(memcmp((void *)entry->code, &code, 5) != 0);
+			if (unlikely(memcmp((void *)entry->code, &code, 5) != 0))
+				bug_at((void *)entry->code, __LINE__);
 		}
 		memcpy(&code, ideal_nops[NOP_ATOMIC5], JUMP_LABEL_NOP_SIZE);
 	}
-- 
1.7.10.4

Re: [PATCH 4/4] x86/jump-label: Show where and what was wrong on errors

[Borislav Petkov]

On Wed, Aug 07, 2013 at 12:49:38PM -0400, Steven Rostedt wrote:
> From: Steven Rostedt <srostedt@redhat.com>
> 
> When modifying text sections for jump labels, a paranoid check is
> performed. If the check fails, the system "bugs". But why it failed
> is not shown.
> 
> The BUG_ON()s in the jump label update code is replaced with bug_at(ip).
> This is a function that will show what pointer failed, and what was
> at the location of the failure that made jump label panic.
> 
> Signed-off-by: Steven Rostedt <rostedt@goodmis.org>
> ---
>  arch/x86/kernel/jump_label.c |   21 ++++++++++++++++++---
>  1 file changed, 18 insertions(+), 3 deletions(-)
> 
> diff --git a/arch/x86/kernel/jump_label.c b/arch/x86/kernel/jump_label.c
> index 24cf2b2..912a528 100644
> --- a/arch/x86/kernel/jump_label.c
> +++ b/arch/x86/kernel/jump_label.c
> @@ -24,6 +24,18 @@ union jump_code_union {
>  	} __attribute__((packed));
>  };
>  
> +static void bug_at(unsigned char *ip, int line)
> +{
> +	/*
> +	 * The location is not an op that we were expecting.
> +	 * Something went wrong. Crash the box, as something could be
> +	 * corrupting the kernel.
> +	 */
> +	pr_warning("Unexpected op at %pS [%p] (%02x %02x %02x %02x %02x) %s:%d\n",
> +	       ip, ip, ip[0], ip[1], ip[2], ip[3], ip[4], __FILE__, line);
> +	BUG();

Why not simply

	panic("Unexpected...")

?

Besides, BUG can be disabled in CONFIG_EXPERT.

-- 
Regards/Gruss,
    Boris.

Sent from a fat crate under my desk. Formatting is fine.
--

Re: [PATCH 4/4] x86/jump-label: Show where and what was wrong on errors

[Steven Rostedt]

On Wed, 2013-08-07 at 19:20 +0200, Borislav Petkov wrote:
>   
> > +static void bug_at(unsigned char *ip, int line)
> > +{
> > +	/*
> > +	 * The location is not an op that we were expecting.
> > +	 * Something went wrong. Crash the box, as something could be
> > +	 * corrupting the kernel.
> > +	 */
> > +	pr_warning("Unexpected op at %pS [%p] (%02x %02x %02x %02x %02x) %s:%d\n",
> > +	       ip, ip, ip[0], ip[1], ip[2], ip[3], ip[4], __FILE__, line);
> > +	BUG();
> 
> Why not simply
> 
> 	panic("Unexpected...")
> 
> ?

Because the patch replaced BUG().

> 
> Besides, BUG can be disabled in CONFIG_EXPERT.
> 

Right, and this code keeps the same logic as it was before. If it was
disabled by CONFIG_EXPERT, it stays disabled, but at least you get to
see a warning that your kernel may be corrupt now :-)

-- Steve

Re: [PATCH 4/4] x86/jump-label: Show where and what was wrong on errors

[Borislav Petkov]

On Wed, Aug 07, 2013 at 01:33:06PM -0400, Steven Rostedt wrote:
> Right, and this code keeps the same logic as it was before. If it was
> disabled by CONFIG_EXPERT, it stays disabled, but at least you get to
> see a warning that your kernel may be corrupt now :-)

Don't we really want to panic instead of running a corrupt kernel? IOW,
to change the logic to panic unconditionally because the image in memory
has been violated and not in a good way, at that :-)

-- 
Regards/Gruss,
    Boris.

Sent from a fat crate under my desk. Formatting is fine.
--

Re: [PATCH 4/4] x86/jump-label: Show where and what was wrong on errors

[Steven Rostedt]

On Wed, 2013-08-07 at 19:37 +0200, Borislav Petkov wrote:
> On Wed, Aug 07, 2013 at 01:33:06PM -0400, Steven Rostedt wrote:
> > Right, and this code keeps the same logic as it was before. If it was
> > disabled by CONFIG_EXPERT, it stays disabled, but at least you get to
> > see a warning that your kernel may be corrupt now :-)
> 
> Don't we really want to panic instead of running a corrupt kernel? IOW,
> to change the logic to panic unconditionally because the image in memory
> has been violated and not in a good way, at that :-)

Well, there's lots of places that use BUG() for a corrupt kernel. If you
are stupid enough to disable it, you get what you asked for.

-- Steve

Re: [PATCH 4/4] x86/jump-label: Show where and what was wrong on errors

[H. Peter Anvin]

On 08/07/2013 10:46 AM, Steven Rostedt wrote:
> On Wed, 2013-08-07 at 19:37 +0200, Borislav Petkov wrote:
>> On Wed, Aug 07, 2013 at 01:33:06PM -0400, Steven Rostedt wrote:
>>> Right, and this code keeps the same logic as it was before. If it was
>>> disabled by CONFIG_EXPERT, it stays disabled, but at least you get to
>>> see a warning that your kernel may be corrupt now :-)
>>
>> Don't we really want to panic instead of running a corrupt kernel? IOW,
>> to change the logic to panic unconditionally because the image in memory
>> has been violated and not in a good way, at that :-)
> 
> Well, there's lots of places that use BUG() for a corrupt kernel. If you
> are stupid enough to disable it, you get what you asked for.
> 

A bigger issue is probably if panic-on-bug should be the default, with
!panic being an opt-in debugging option.

	-hpa

Re: [PATCH 4/4] x86/jump-label: Show where and what was wrong on errors

[Borislav Petkov]

On Wed, Aug 07, 2013 at 10:51:43AM -0700, H. Peter Anvin wrote:
> A bigger issue is probably if panic-on-bug should be the default, with
> !panic being an opt-in debugging option.

Yes, it might make sense although embedded wants to disable CONFIG_BUG
on systems which cannot report errors:

  │ CONFIG_BUG:
  │
  │ Disabling this option eliminates support for BUG and WARN, reducing
  │ the size of your kernel image and potentially quietly ignoring
  │ numerous fatal conditions. You should only consider disabling this
  │ option for embedded systems with no facilities for reporting errors.
  │ Just say Y.

-- 
Regards/Gruss,
    Boris.

Sent from a fat crate under my desk. Formatting is fine.
--

Re: [PATCH 4/4] x86/jump-label: Show where and what was wrong on errors

[H. Peter Anvin]

On 08/07/2013 11:41 AM, Borislav Petkov wrote:
> On Wed, Aug 07, 2013 at 10:51:43AM -0700, H. Peter Anvin wrote:
>> A bigger issue is probably if panic-on-bug should be the default, with
>> !panic being an opt-in debugging option.
> 
> Yes, it might make sense although embedded wants to disable CONFIG_BUG
> on systems which cannot report errors:
> 

There is another thread going on to convert BUG to trap even on machines
where CONFIG_BUG is disabled, i.e. CONFIG_BUG would control the metadata
only.  With the configuration option off, you would get the address of
the failure only.

	-hpa

Re: [PATCH 4/4] x86/jump-label: Show where and what was wrong on errors

[Peter Zijlstra]

On Wed, Aug 07, 2013 at 07:20:44PM +0200, Borislav Petkov wrote:
> Besides, BUG can be disabled in CONFIG_EXPERT.

There was some email on this subject a while ago; disabling BUG() is
very risky and can cause all kinds of horrid. IIRC the consensus back
then was to remove this 'feature' and have BUG always at least lock up
the machine hard.

Re: [PATCH 4/4] x86/jump-label: Show where and what was wrong on errors

[H. Peter Anvin]

On 08/12/2013 07:31 AM, Peter Zijlstra wrote:
> On Wed, Aug 07, 2013 at 07:20:44PM +0200, Borislav Petkov wrote:
>> Besides, BUG can be disabled in CONFIG_EXPERT.
> 
> There was some email on this subject a while ago; disabling BUG() is
> very risky and can cause all kinds of horrid. IIRC the consensus back
> then was to remove this 'feature' and have BUG always at least lock up
> the machine hard.
> 

Yes, always trap but leave the metadata as optional.

	-hpa

Re: [PATCH 4/4] x86/jump-label: Show where and what was wrong on errors

[Ingo Molnar]

* H. Peter Anvin <hpa@zytor.com> wrote:

> On 08/12/2013 07:31 AM, Peter Zijlstra wrote:
> > On Wed, Aug 07, 2013 at 07:20:44PM +0200, Borislav Petkov wrote:
> >> Besides, BUG can be disabled in CONFIG_EXPERT.
> > 
> > There was some email on this subject a while ago; disabling BUG() is 
> > very risky and can cause all kinds of horrid. IIRC the consensus back 
> > then was to remove this 'feature' and have BUG always at least lock up 
> > the machine hard.
> 
> Yes, always trap but leave the metadata as optional.

I think those patches are going forward, so we can just regard BUG() as 
always existing from a program flow POV.

Thanks,

	Ingo