* [PATCH v2] x86: Stop relying on magic jmp behavior for early_idt_handlers
@ 2015-05-22 1:27 Andy Lutomirski
2015-05-22 6:22 ` Ingo Molnar
0 siblings, 1 reply; 5+ messages in thread
From: Andy Lutomirski @ 2015-05-22 1:27 UTC (permalink / raw)
To: x86, H. Peter Anvin, H.J. Lu
Cc: Borislav Petkov, Jan Beulich, Binutils, linux-kernel, 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>
---
Conditionally-Acked-by: H. Peter Anvin <hpa@linux.intel.com>
Changes from v1:
- Changed .globl to ENTRY.
- Removed superfluous endif and ifdef
arch/x86/include/asm/segment.h | 10 +++++++++-
arch/x86/kernel/head_32.S | 12 ++++++------
arch/x86/kernel/head_64.S | 14 +++++++-------
3 files changed, 22 insertions(+), 14 deletions(-)
diff --git a/arch/x86/include/asm/segment.h b/arch/x86/include/asm/segment.h
index 5a9856eb12ba..15f5bfbb24ef 100644
--- a/arch/x86/include/asm/segment.h
+++ b/arch/x86/include/asm/segment.h
@@ -231,9 +231,17 @@
#define TLS_SIZE (GDT_ENTRY_TLS_ENTRIES* 8)
#ifdef __KERNEL__
+
+/*
+ * 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][2+2+5];
+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
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..af4e3b26133b 100644
--- a/arch/x86/kernel/head_64.S
+++ b/arch/x86/kernel/head_64.S
@@ -320,27 +320,27 @@ ENDPROC(start_cpu0)
bad_address:
jmp bad_address
+/* Build the early_idt_handlers array */
__INIT
- .globl early_idt_handlers
-early_idt_handlers:
+ENTRY(early_idt_handlers)
# 104(%rsp) %rflags
# 96(%rsp) %cs
# 88(%rsp) %rip
# 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
^ permalink raw reply related [flat|nested] 5+ messages in thread
* Re: [PATCH v2] x86: Stop relying on magic jmp behavior for early_idt_handlers
2015-05-22 1:27 [PATCH v2] x86: Stop relying on magic jmp behavior for early_idt_handlers Andy Lutomirski
@ 2015-05-22 6:22 ` Ingo Molnar
2015-05-22 6:51 ` Andy Lutomirski
0 siblings, 1 reply; 5+ messages in thread
From: Ingo Molnar @ 2015-05-22 6:22 UTC (permalink / raw)
To: Andy Lutomirski
Cc: x86, H. Peter Anvin, H.J. Lu, Borislav Petkov, Jan Beulich,
Binutils, linux-kernel
* Andy Lutomirski <luto@kernel.org> wrote:
> --- a/arch/x86/include/asm/segment.h
> +++ b/arch/x86/include/asm/segment.h
> @@ -231,9 +231,17 @@
> #define TLS_SIZE (GDT_ENTRY_TLS_ENTRIES* 8)
>
> #ifdef __KERNEL__
> +
> +/*
> + * 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
So how come that two plus five equals nine? ;-)
It's two two-bype pushes (sometimes a 2-byte NOP), plus a 5-byte jump,
in the worst case.
I'd also mention that it's an array of 32 small trampolines that set
up parameters and jump to a common entry point.
> +/* Build the early_idt_handlers array */
> ENTRY(early_idt_handlers)
Please rename the function accordingly: early_idt_handlers_array, to
make clear this is never jumped to directly, only through the IDT.
> # 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:
Please rename this as well to make it clearer what's happening,
something like:
early_idt_handler_common:
would work for me.
Ditto for the 64-bit side.
Thanks,
Ingo
^ permalink raw reply [flat|nested] 5+ messages in thread
* Re: [PATCH v2] x86: Stop relying on magic jmp behavior for early_idt_handlers
2015-05-22 6:22 ` Ingo Molnar
@ 2015-05-22 6:51 ` Andy Lutomirski
2015-05-22 20:59 ` H. Peter Anvin
2015-05-22 20:59 ` H. Peter Anvin
0 siblings, 2 replies; 5+ messages in thread
From: Andy Lutomirski @ 2015-05-22 6:51 UTC (permalink / raw)
To: Ingo Molnar
Cc: Andy Lutomirski, X86 ML, H. Peter Anvin, H.J. Lu,
Borislav Petkov, Jan Beulich, Binutils, linux-kernel
On Thu, May 21, 2015 at 11:22 PM, Ingo Molnar <mingo@kernel.org> wrote:
>
> * Andy Lutomirski <luto@kernel.org> wrote:
>
>> --- a/arch/x86/include/asm/segment.h
>> +++ b/arch/x86/include/asm/segment.h
>> @@ -231,9 +231,17 @@
>> #define TLS_SIZE (GDT_ENTRY_TLS_ENTRIES* 8)
>>
>> #ifdef __KERNEL__
>> +
>> +/*
>> + * 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
>
> So how come that two plus five equals nine? ;-)
A "one-byte push" would be pretty impressive :)
>
> It's two two-bype pushes (sometimes a 2-byte NOP), plus a 5-byte jump,
> in the worst case.
>
> I'd also mention that it's an array of 32 small trampolines that set
> up parameters and jump to a common entry point.
Will do.
>
>> +/* Build the early_idt_handlers array */
>> ENTRY(early_idt_handlers)
>
> Please rename the function accordingly: early_idt_handlers_array, to
> make clear this is never jumped to directly, only through the IDT.
Makes sense.
>
>> # 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:
>
> Please rename this as well to make it clearer what's happening,
> something like:
>
> early_idt_handler_common:
>
> would work for me.
>
> Ditto for the 64-bit side.
Sounds good. I'll do all this tomorrow and resend.
--Andy
^ permalink raw reply [flat|nested] 5+ messages in thread
* Re: [PATCH v2] x86: Stop relying on magic jmp behavior for early_idt_handlers
2015-05-22 6:51 ` Andy Lutomirski
@ 2015-05-22 20:59 ` H. Peter Anvin
2015-05-22 20:59 ` H. Peter Anvin
1 sibling, 0 replies; 5+ messages in thread
From: H. Peter Anvin @ 2015-05-22 20:59 UTC (permalink / raw)
To: Andy Lutomirski, Ingo Molnar
Cc: Andy Lutomirski, X86 ML, H.J. Lu, Borislav Petkov, Jan Beulich,
Binutils, linux-kernel
On 05/21/2015 11:51 PM, Andy Lutomirski wrote:
>>>
>>> #ifdef __KERNEL__
>>> +
>>> +/*
>>> + * 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
>>
>> So how come that two plus five equals nine? ;-)
>
> A "one-byte push" would be pretty impressive :)
>
Well, non-REX register pushes are just one byte. However, these take an
immediate constant.
>>
>> Ditto for the 64-bit side.
>
> Sounds good. I'll do all this tomorrow and resend.
>
Thank you! Since I trust you, I'll send you this in advance:
Acked-by: H. Peter Anvin <hpa@linux.intel.com>
^ permalink raw reply [flat|nested] 5+ messages in thread
* Re: [PATCH v2] x86: Stop relying on magic jmp behavior for early_idt_handlers
2015-05-22 6:51 ` Andy Lutomirski
2015-05-22 20:59 ` H. Peter Anvin
@ 2015-05-22 20:59 ` H. Peter Anvin
1 sibling, 0 replies; 5+ messages in thread
From: H. Peter Anvin @ 2015-05-22 20:59 UTC (permalink / raw)
To: Andy Lutomirski, Ingo Molnar
Cc: Andy Lutomirski, X86 ML, H.J. Lu, Borislav Petkov, Jan Beulich,
Binutils, linux-kernel
On 05/21/2015 11:51 PM, Andy Lutomirski wrote:
>>>
>>> #ifdef __KERNEL__
>>> +
>>> +/*
>>> + * 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
>>
>> So how come that two plus five equals nine? ;-)
>
> A "one-byte push" would be pretty impressive :)
>
Well, non-REX register pushes are just one byte. However, these take an
immediate constant.
>>
>> Ditto for the 64-bit side.
>
> Sounds good. I'll do all this tomorrow and resend.
>
Thank you! Since I trust you, I'll send you this in advance:
Acked-by: H. Peter Anvin <hpa@linux.intel.com>
^ permalink raw reply [flat|nested] 5+ messages in thread
end of thread, other threads:[~2015-05-22 21:00 UTC | newest]
Thread overview: 5+ messages (download: mbox.gz / follow: Atom feed)
-- links below jump to the message on this page --
2015-05-22 1:27 [PATCH v2] x86: Stop relying on magic jmp behavior for early_idt_handlers Andy Lutomirski
2015-05-22 6:22 ` Ingo Molnar
2015-05-22 6:51 ` Andy Lutomirski
2015-05-22 20:59 ` H. Peter Anvin
2015-05-22 20:59 ` H. Peter Anvin
This is an external index of several public inboxes,
see mirroring instructions on how to clone and mirror
all data and code used by this external index.