[PATCH] x86: Stop relying on magic jmp behavior for early_idt_handlers

[PATCH] x86: Stop relying on magic jmp behavior for early_idt_handlers

[Andy Lutomirski]

The early_idt_handlers asm code generates an array of entry points
spaced nine bytes apart.  It's not really clear from that code
or from the places that reference it what's going on, and the code
only works in the first place because gas never generates two-byte
jmp instructions when jumping to global labels.

Clean up the code to generate the correct array stride explicitly.
This should be considerably more robust against screw-ups, as gas
will warn if a .fill directive has a negative count.  Using '. =' to
advance would have been even more robust (it would generate an
actual error if it tried to move backwards), but it would pad with
nulls, confusing anyone who tries to disassemble the code.  The new
scheme should be much clearer to future readers.

Binutils may start relaxing jumps to non-weak labels.  If so, this
change will fix our build, and we may need to backport this change.

Before, on x86_64:

0000000000000000 <early_idt_handlers>:
   0:   6a 00                   pushq  $0x0
   2:   6a 00                   pushq  $0x0
   4:   e9 00 00 00 00          jmpq   9 <early_idt_handlers+0x9>
                        5: R_X86_64_PC32        early_idt_handler-0x4
...
  48:   66 90                   xchg   %ax,%ax
  4a:   6a 08                   pushq  $0x8
  4c:   e9 00 00 00 00          jmpq   51 <early_idt_handlers+0x51>
                        4d: R_X86_64_PC32       early_idt_handler-0x4
...
 117:   6a 00                   pushq  $0x0
 119:   6a 1f                   pushq  $0x1f
 11b:   e9 00 00 00 00          jmpq   120 <early_idt_handler>
                        11c: R_X86_64_PC32      early_idt_handler-0x4

After:

0000000000000000 <early_idt_handlers>:
   0:   6a 00                   pushq  $0x0
   2:   6a 00                   pushq  $0x0
   4:   e9 14 01 00 00          jmpq   11d <early_idt_handler>
...
  48:   6a 08                   pushq  $0x8
  4a:   e9 d1 00 00 00          jmpq   120 <early_idt_handler>
  4f:   cc                      int3
  50:   cc                      int3
...
 117:   6a 00                   pushq  $0x0
 119:   6a 1f                   pushq  $0x1f
 11b:   eb 03                   jmp    120 <early_idt_handler>
 11d:   cc                      int3
 11e:   cc                      int3
 11f:   cc                      int3

Signed-off-by: Andy Lutomirski <luto@kernel.org>
---
 arch/x86/include/asm/segment.h | 13 +++++++++++--
 arch/x86/kernel/head_32.S      | 12 ++++++------
 arch/x86/kernel/head_64.S      | 11 ++++++-----
 3 files changed, 23 insertions(+), 13 deletions(-)

diff --git a/arch/x86/include/asm/segment.h b/arch/x86/include/asm/segment.h
index 5a9856eb12ba..4bbe0eb24d7e 100644
--- a/arch/x86/include/asm/segment.h
+++ b/arch/x86/include/asm/segment.h
@@ -231,12 +231,21 @@
 #define TLS_SIZE			(GDT_ENTRY_TLS_ENTRIES* 8)
 
 #ifdef __KERNEL__
-#ifndef __ASSEMBLY__
 
-extern const char early_idt_handlers[NUM_EXCEPTION_VECTORS][2+2+5];
+/*
+ * early_idt_handlers is an array of entry points.  For simplicity, it's
+ * a real array.  We allocate nine bytes for each entry: two one-byte
+ * push instructions and a five-byte jump in the worst case.
+ */
+#define EARLY_IDT_HANDLER_STRIDE 9
+#ifndef __ASSEMBLY__
+extern const char early_idt_handlers[NUM_EXCEPTION_VECTORS][EARLY_IDT_HANDLER_STRIDE];
 #ifdef CONFIG_TRACING
 # define trace_early_idt_handlers early_idt_handlers
 #endif
+#endif
+
+#ifndef __ASSEMBLY__
 
 /*
  * Load a segment. Fall back on loading the zero
diff --git a/arch/x86/kernel/head_32.S b/arch/x86/kernel/head_32.S
index d031bad9e07e..43154cd4ad62 100644
--- a/arch/x86/kernel/head_32.S
+++ b/arch/x86/kernel/head_32.S
@@ -492,7 +492,7 @@ setup_once:
 	movl %eax,4(%edi)
 	/* interrupt gate, dpl=0, present */
 	movl $(0x8E000000 + __KERNEL_CS),2(%edi)
-	addl $9,%eax
+	addl $EARLY_IDT_HANDLER_STRIDE,%eax
 	addl $8,%edi
 	loop 1b
 
@@ -524,6 +524,7 @@ setup_once:
 	andl $0,setup_once_ref	/* Once is enough, thanks */
 	ret
 
+/* Build the early_idt_handlers array */
 ENTRY(early_idt_handlers)
 	# 36(%esp) %eflags
 	# 32(%esp) %cs
@@ -531,19 +532,18 @@ ENTRY(early_idt_handlers)
 	# 24(%rsp) error code
 	i = 0
 	.rept NUM_EXCEPTION_VECTORS
-	.if (EXCEPTION_ERRCODE_MASK >> i) & 1
-	ASM_NOP2
-	.else
+	.fill early_idt_handlers + i * EARLY_IDT_HANDLER_STRIDE - ., 1, 0xcc
+	.ifeq (EXCEPTION_ERRCODE_MASK >> i) & 1
 	pushl $0		# Dummy error code, to make stack frame uniform
 	.endif
 	pushl $i		# 20(%esp) Vector number
 	jmp early_idt_handler
 	i = i + 1
 	.endr
+	.fill early_idt_handlers + i * EARLY_IDT_HANDLER_STRIDE - ., 1, 0xcc
 ENDPROC(early_idt_handlers)
 	
-	/* This is global to keep gas from relaxing the jumps */
-ENTRY(early_idt_handler)
+early_idt_handler:
 	cld
 
 	cmpl $2,(%esp)		# X86_TRAP_NMI
diff --git a/arch/x86/kernel/head_64.S b/arch/x86/kernel/head_64.S
index ae6588b301c2..c1874059aadf 100644
--- a/arch/x86/kernel/head_64.S
+++ b/arch/x86/kernel/head_64.S
@@ -320,6 +320,7 @@ ENDPROC(start_cpu0)
 bad_address:
 	jmp bad_address
 
+/* Build the early_idt_handlers array */
 	__INIT
 	.globl early_idt_handlers
 early_idt_handlers:
@@ -329,18 +330,18 @@ early_idt_handlers:
 	#  80(%rsp) error code
 	i = 0
 	.rept NUM_EXCEPTION_VECTORS
-	.if (EXCEPTION_ERRCODE_MASK >> i) & 1
-	ASM_NOP2
-	.else
+	.fill early_idt_handlers + i * EARLY_IDT_HANDLER_STRIDE - ., 1, 0xcc
+	.ifeq (EXCEPTION_ERRCODE_MASK >> i) & 1
 	pushq $0		# Dummy error code, to make stack frame uniform
 	.endif
 	pushq $i		# 72(%rsp) Vector number
 	jmp early_idt_handler
 	i = i + 1
 	.endr
+	.fill early_idt_handlers + i * EARLY_IDT_HANDLER_STRIDE - ., 1, 0xcc
+ENDPROC(early_idt_handlers)
 
-/* This is global to keep gas from relaxing the jumps */
-ENTRY(early_idt_handler)
+early_idt_handler:
 	cld
 
 	cmpl $2,(%rsp)		# X86_TRAP_NMI
-- 
2.3.0

Re: [PATCH] x86: Stop relying on magic jmp behavior for early_idt_handlers

[H. Peter Anvin]

On 05/21/2015 05:17 PM, Andy Lutomirski wrote:
> 
> diff --git a/arch/x86/include/asm/segment.h b/arch/x86/include/asm/segment.h
> index 5a9856eb12ba..4bbe0eb24d7e 100644
> --- a/arch/x86/include/asm/segment.h
> +++ b/arch/x86/include/asm/segment.h
> @@ -231,12 +231,21 @@
>  #define TLS_SIZE			(GDT_ENTRY_TLS_ENTRIES* 8)
>  
>  #ifdef __KERNEL__
> -#ifndef __ASSEMBLY__
>  
> -extern const char early_idt_handlers[NUM_EXCEPTION_VECTORS][2+2+5];
> +/*
> + * early_idt_handlers is an array of entry points.  For simplicity, it's
> + * a real array.  We allocate nine bytes for each entry: two one-byte
> + * push instructions and a five-byte jump in the worst case.
> + */
> +#define EARLY_IDT_HANDLER_STRIDE 9
> +#ifndef __ASSEMBLY__
> +extern const char early_idt_handlers[NUM_EXCEPTION_VECTORS][EARLY_IDT_HANDLER_STRIDE];
>  #ifdef CONFIG_TRACING
>  # define trace_early_idt_handlers early_idt_handlers
>  #endif
> +#endif
> +
> +#ifndef __ASSEMBLY__
>  

Please get rid of the unnecessary #ifndef/#endif pair.

> +/* Build the early_idt_handlers array */
>  	__INIT
>  	.globl early_idt_handlers

Let's use ENTRY(early_idt_handlers) for consistency.

Otherwise it looks good.  I like the use of .fill better than moving .,
it seems more robust to me.

Conditionally-Acked-by: H. Peter Anvin <hpa@linux.intel.com>

Re: [PATCH] x86: Stop relying on magic jmp behavior for early_idt_handlers

[Andy Lutomirski]

On Thu, May 21, 2015 at 5:32 PM, H. Peter Anvin <hpa@zytor.com> wrote:
> On 05/21/2015 05:17 PM, Andy Lutomirski wrote:
>>
>> diff --git a/arch/x86/include/asm/segment.h b/arch/x86/include/asm/segment.h
>> index 5a9856eb12ba..4bbe0eb24d7e 100644
>> --- a/arch/x86/include/asm/segment.h
>> +++ b/arch/x86/include/asm/segment.h
>> @@ -231,12 +231,21 @@
>>  #define TLS_SIZE                     (GDT_ENTRY_TLS_ENTRIES* 8)
>>
>>  #ifdef __KERNEL__
>> -#ifndef __ASSEMBLY__
>>
>> -extern const char early_idt_handlers[NUM_EXCEPTION_VECTORS][2+2+5];
>> +/*
>> + * early_idt_handlers is an array of entry points.  For simplicity, it's
>> + * a real array.  We allocate nine bytes for each entry: two one-byte
>> + * push instructions and a five-byte jump in the worst case.
>> + */
>> +#define EARLY_IDT_HANDLER_STRIDE 9
>> +#ifndef __ASSEMBLY__
>> +extern const char early_idt_handlers[NUM_EXCEPTION_VECTORS][EARLY_IDT_HANDLER_STRIDE];
>>  #ifdef CONFIG_TRACING
>>  # define trace_early_idt_handlers early_idt_handlers
>>  #endif
>> +#endif
>> +
>> +#ifndef __ASSEMBLY__
>>
>
> Please get rid of the unnecessary #ifndef/#endif pair.

I did it to preserve logical nesting.  I didn't want the pile of
early_idt_handlers declarations and comments to have the side affect
of starting an #ifdef block.

If you still think I should change it, I'll change it.

>
>> +/* Build the early_idt_handlers array */
>>       __INIT
>>       .globl early_idt_handlers
>
> Let's use ENTRY(early_idt_handlers) for consistency.

Will do.

--Andy

>
> Otherwise it looks good.  I like the use of .fill better than moving .,
> it seems more robust to me.
>
> Conditionally-Acked-by: H. Peter Anvin <hpa@linux.intel.com>
>



-- 
Andy Lutomirski
AMA Capital Management, LLC

Re: [PATCH] x86: Stop relying on magic jmp behavior for early_idt_handlers

[H. Peter Anvin]

On 05/21/2015 05:38 PM, Andy Lutomirski wrote:
> On Thu, May 21, 2015 at 5:32 PM, H. Peter Anvin <hpa@zytor.com> wrote:
>> On 05/21/2015 05:17 PM, Andy Lutomirski wrote:
>>>
>>> diff --git a/arch/x86/include/asm/segment.h b/arch/x86/include/asm/segment.h
>>> index 5a9856eb12ba..4bbe0eb24d7e 100644
>>> --- a/arch/x86/include/asm/segment.h
>>> +++ b/arch/x86/include/asm/segment.h
>>> @@ -231,12 +231,21 @@
>>>  #define TLS_SIZE                     (GDT_ENTRY_TLS_ENTRIES* 8)
>>>
>>>  #ifdef __KERNEL__
>>> -#ifndef __ASSEMBLY__
>>>
>>> -extern const char early_idt_handlers[NUM_EXCEPTION_VECTORS][2+2+5];
>>> +/*
>>> + * early_idt_handlers is an array of entry points.  For simplicity, it's
>>> + * a real array.  We allocate nine bytes for each entry: two one-byte
>>> + * push instructions and a five-byte jump in the worst case.
>>> + */
>>> +#define EARLY_IDT_HANDLER_STRIDE 9
>>> +#ifndef __ASSEMBLY__
>>> +extern const char early_idt_handlers[NUM_EXCEPTION_VECTORS][EARLY_IDT_HANDLER_STRIDE];
>>>  #ifdef CONFIG_TRACING
>>>  # define trace_early_idt_handlers early_idt_handlers
>>>  #endif
>>> +#endif
>>> +
>>> +#ifndef __ASSEMBLY__
>>>
>>
>> Please get rid of the unnecessary #ifndef/#endif pair.
> 
> I did it to preserve logical nesting.  I didn't want the pile of
> early_idt_handlers declarations and comments to have the side affect
> of starting an #ifdef block.
> 
> If you still think I should change it, I'll change it.
> 

Please change it.  We generally don't do that kind of logical nesting,
it is just clutter.

	-hpa