[PATCH v5] x86: make sure IDT is page aligned

[PATCH v5] x86: make sure IDT is page aligned

[Kees Cook]

Since the IDT is referenced from a fixmap, make sure it is page aligned.
Merge with 32-bit one, since it was already aligned to deal with F00F
bug. Since bss is cleared before IDT setup, it can live there. This also
moves the other *_idt_table variables into common locations.

This avoids the risk of the IDT ever being moved in the bss and having
the mapping be offset, resulting in calling incorrect handlers. In the
current upstream kernel this is not a manifested bug, but heavily patched
kernels (such as those using the PaX patch series) did encounter this bug.

Signed-off-by: Kees Cook <keescook@chromium.org>
Reported-by: PaX Team <pageexec@gmail.com>
---
v5:
 - add comments to all IDTs about alignment reasoning, suggested by Linus
v4:
 - rework using __page_aligned_bss, suggested by Yinghai LU
 - move all the other IDT variables as well, suggested by HPA
v3:
 - merge 32-bit and 64-bit idt_table definition
v2:
 - 32-bit was already aligned
---
 arch/x86/kernel/head_64.S    |   15 ---------------
 arch/x86/kernel/tracepoint.c |    6 ++----
 arch/x86/kernel/traps.c      |   12 ++++++------
 3 files changed, 8 insertions(+), 25 deletions(-)

diff --git a/arch/x86/kernel/head_64.S b/arch/x86/kernel/head_64.S
index 5e4d8a8..e1aabdb 100644
--- a/arch/x86/kernel/head_64.S
+++ b/arch/x86/kernel/head_64.S
@@ -512,21 +512,6 @@ ENTRY(phys_base)
 
 #include "../../x86/xen/xen-head.S"
 	
-	.section .bss, "aw", @nobits
-	.align L1_CACHE_BYTES
-ENTRY(idt_table)
-	.skip IDT_ENTRIES * 16
-
-	.align L1_CACHE_BYTES
-ENTRY(debug_idt_table)
-	.skip IDT_ENTRIES * 16
-
-#ifdef CONFIG_TRACING
-	.align L1_CACHE_BYTES
-ENTRY(trace_idt_table)
-	.skip IDT_ENTRIES * 16
-#endif
-
 	__PAGE_ALIGNED_BSS
 NEXT_PAGE(empty_zero_page)
 	.skip PAGE_SIZE
diff --git a/arch/x86/kernel/tracepoint.c b/arch/x86/kernel/tracepoint.c
index 4e584a8..1c113db 100644
--- a/arch/x86/kernel/tracepoint.c
+++ b/arch/x86/kernel/tracepoint.c
@@ -12,10 +12,8 @@ atomic_t trace_idt_ctr = ATOMIC_INIT(0);
 struct desc_ptr trace_idt_descr = { NR_VECTORS * 16 - 1,
 				(unsigned long) trace_idt_table };
 
-#ifndef CONFIG_X86_64
-gate_desc trace_idt_table[NR_VECTORS] __page_aligned_data
-					= { { { { 0, 0 } } }, };
-#endif
+/* No need to be aligned, but done to keep all IDTs defined the same way. */
+gate_desc trace_idt_table[NR_VECTORS] __page_aligned_bss;
 
 static int trace_irq_vector_refcount;
 static DEFINE_MUTEX(irq_vector_mutex);
diff --git a/arch/x86/kernel/traps.c b/arch/x86/kernel/traps.c
index b0865e8..1b23a1c 100644
--- a/arch/x86/kernel/traps.c
+++ b/arch/x86/kernel/traps.c
@@ -63,19 +63,19 @@
 #include <asm/x86_init.h>
 #include <asm/pgalloc.h>
 #include <asm/proto.h>
+
+/* No need to be aligned, but done to keep all IDTs defined the same way. */
+gate_desc debug_idt_table[NR_VECTORS] __page_aligned_bss;
 #else
 #include <asm/processor-flags.h>
 #include <asm/setup.h>
 
 asmlinkage int system_call(void);
-
-/*
- * The IDT has to be page-aligned to simplify the Pentium
- * F0 0F bug workaround.
- */
-gate_desc idt_table[NR_VECTORS] __page_aligned_data = { { { { 0, 0 } } }, };
 #endif
 
+/* Must be page-aligned because the real IDT is used in a fixmap. */
+gate_desc idt_table[NR_VECTORS] __page_aligned_bss;
+
 DECLARE_BITMAP(used_vectors, NR_VECTORS);
 EXPORT_SYMBOL_GPL(used_vectors);
 
-- 
1.7.9.5


-- 
Kees Cook
Chrome OS Security

Re: [PATCH v5] x86: make sure IDT is page aligned

[Steven Rostedt]

On Tue, 2013-07-16 at 11:34 -0700, Kees Cook wrote:
> Since the IDT is referenced from a fixmap, make sure it is page aligned.
> Merge with 32-bit one, since it was already aligned to deal with F00F
> bug. Since bss is cleared before IDT setup, it can live there. This also
> moves the other *_idt_table variables into common locations.
> 
> This avoids the risk of the IDT ever being moved in the bss and having
> the mapping be offset, resulting in calling incorrect handlers. In the
> current upstream kernel this is not a manifested bug, but heavily patched
> kernels (such as those using the PaX patch series) did encounter this bug.
> 
> Signed-off-by: Kees Cook <keescook@chromium.org>
> Reported-by: PaX Team <pageexec@gmail.com>


I like it just because it removes those nasty #ifdef blocks for x86_64
or not.

Acked-by: Steven Rostedt <rostedt@goodmis.org>

-- Steve

> ---
> v5:
>  - add comments to all IDTs about alignment reasoning, suggested by Linus
> v4:
>  - rework using __page_aligned_bss, suggested by Yinghai LU
>  - move all the other IDT variables as well, suggested by HPA
> v3:
>  - merge 32-bit and 64-bit idt_table definition
> v2:
>  - 32-bit was already aligned
> ---
>  arch/x86/kernel/head_64.S    |   15 ---------------
>  arch/x86/kernel/tracepoint.c |    6 ++----
>  arch/x86/kernel/traps.c      |   12 ++++++------
>  3 files changed, 8 insertions(+), 25 deletions(-)
> 
> diff --git a/arch/x86/kernel/head_64.S b/arch/x86/kernel/head_64.S
> index 5e4d8a8..e1aabdb 100644
> --- a/arch/x86/kernel/head_64.S
> +++ b/arch/x86/kernel/head_64.S
> @@ -512,21 +512,6 @@ ENTRY(phys_base)
>  
>  #include "../../x86/xen/xen-head.S"
>  	
> -	.section .bss, "aw", @nobits
> -	.align L1_CACHE_BYTES
> -ENTRY(idt_table)
> -	.skip IDT_ENTRIES * 16
> -
> -	.align L1_CACHE_BYTES
> -ENTRY(debug_idt_table)
> -	.skip IDT_ENTRIES * 16
> -
> -#ifdef CONFIG_TRACING
> -	.align L1_CACHE_BYTES
> -ENTRY(trace_idt_table)
> -	.skip IDT_ENTRIES * 16
> -#endif
> -
>  	__PAGE_ALIGNED_BSS
>  NEXT_PAGE(empty_zero_page)
>  	.skip PAGE_SIZE
> diff --git a/arch/x86/kernel/tracepoint.c b/arch/x86/kernel/tracepoint.c
> index 4e584a8..1c113db 100644
> --- a/arch/x86/kernel/tracepoint.c
> +++ b/arch/x86/kernel/tracepoint.c
> @@ -12,10 +12,8 @@ atomic_t trace_idt_ctr = ATOMIC_INIT(0);
>  struct desc_ptr trace_idt_descr = { NR_VECTORS * 16 - 1,
>  				(unsigned long) trace_idt_table };
>  
> -#ifndef CONFIG_X86_64
> -gate_desc trace_idt_table[NR_VECTORS] __page_aligned_data
> -					= { { { { 0, 0 } } }, };
> -#endif
> +/* No need to be aligned, but done to keep all IDTs defined the same way. */
> +gate_desc trace_idt_table[NR_VECTORS] __page_aligned_bss;
>  
>  static int trace_irq_vector_refcount;
>  static DEFINE_MUTEX(irq_vector_mutex);
> diff --git a/arch/x86/kernel/traps.c b/arch/x86/kernel/traps.c
> index b0865e8..1b23a1c 100644
> --- a/arch/x86/kernel/traps.c
> +++ b/arch/x86/kernel/traps.c
> @@ -63,19 +63,19 @@
>  #include <asm/x86_init.h>
>  #include <asm/pgalloc.h>
>  #include <asm/proto.h>
> +
> +/* No need to be aligned, but done to keep all IDTs defined the same way. */
> +gate_desc debug_idt_table[NR_VECTORS] __page_aligned_bss;
>  #else
>  #include <asm/processor-flags.h>
>  #include <asm/setup.h>
>  
>  asmlinkage int system_call(void);
> -
> -/*
> - * The IDT has to be page-aligned to simplify the Pentium
> - * F0 0F bug workaround.
> - */
> -gate_desc idt_table[NR_VECTORS] __page_aligned_data = { { { { 0, 0 } } }, };
>  #endif
>  
> +/* Must be page-aligned because the real IDT is used in a fixmap. */
> +gate_desc idt_table[NR_VECTORS] __page_aligned_bss;
> +
>  DECLARE_BITMAP(used_vectors, NR_VECTORS);
>  EXPORT_SYMBOL_GPL(used_vectors);
>  
> -- 
> 1.7.9.5
> 
> 

Re: [PATCH v5] x86: make sure IDT is page aligned

[Yinghai Lu]

On Tue, Jul 16, 2013 at 11:34 AM, Kees Cook <keescook@chromium.org> wrote:
> Since the IDT is referenced from a fixmap, make sure it is page aligned.
> Merge with 32-bit one, since it was already aligned to deal with F00F
> bug. Since bss is cleared before IDT setup, it can live there. This also
> moves the other *_idt_table variables into common locations.
>
> This avoids the risk of the IDT ever being moved in the bss and having
> the mapping be offset, resulting in calling incorrect handlers. In the
> current upstream kernel this is not a manifested bug, but heavily patched
> kernels (such as those using the PaX patch series) did encounter this bug.
>
> Signed-off-by: Kees Cook <keescook@chromium.org>
> Reported-by: PaX Team <pageexec@gmail.com>
> ---
> v5:
>  - add comments to all IDTs about alignment reasoning, suggested by Linus

Where is thread for that?

> v4:
>  - rework using __page_aligned_bss, suggested by Yinghai LU
>  - move all the other IDT variables as well, suggested by HPA
> v3:
>  - merge 32-bit and 64-bit idt_table definition
> v2:
>  - 32-bit was already aligned
> ---
>  arch/x86/kernel/head_64.S    |   15 ---------------
>  arch/x86/kernel/tracepoint.c |    6 ++----
>  arch/x86/kernel/traps.c      |   12 ++++++------
>  3 files changed, 8 insertions(+), 25 deletions(-)
>
> diff --git a/arch/x86/kernel/head_64.S b/arch/x86/kernel/head_64.S
> index 5e4d8a8..e1aabdb 100644
> --- a/arch/x86/kernel/head_64.S
> +++ b/arch/x86/kernel/head_64.S
> @@ -512,21 +512,6 @@ ENTRY(phys_base)
>
>  #include "../../x86/xen/xen-head.S"
>
> -       .section .bss, "aw", @nobits
> -       .align L1_CACHE_BYTES
> -ENTRY(idt_table)
> -       .skip IDT_ENTRIES * 16
> -
> -       .align L1_CACHE_BYTES
> -ENTRY(debug_idt_table)
> -       .skip IDT_ENTRIES * 16
> -
> -#ifdef CONFIG_TRACING
> -       .align L1_CACHE_BYTES
> -ENTRY(trace_idt_table)
> -       .skip IDT_ENTRIES * 16
> -#endif
> -
>         __PAGE_ALIGNED_BSS
>  NEXT_PAGE(empty_zero_page)
>         .skip PAGE_SIZE
> diff --git a/arch/x86/kernel/tracepoint.c b/arch/x86/kernel/tracepoint.c
> index 4e584a8..1c113db 100644
> --- a/arch/x86/kernel/tracepoint.c
> +++ b/arch/x86/kernel/tracepoint.c
> @@ -12,10 +12,8 @@ atomic_t trace_idt_ctr = ATOMIC_INIT(0);
>  struct desc_ptr trace_idt_descr = { NR_VECTORS * 16 - 1,
>                                 (unsigned long) trace_idt_table };
>
> -#ifndef CONFIG_X86_64
> -gate_desc trace_idt_table[NR_VECTORS] __page_aligned_data
> -                                       = { { { { 0, 0 } } }, };
> -#endif
> +/* No need to be aligned, but done to keep all IDTs defined the same way. */
> +gate_desc trace_idt_table[NR_VECTORS] __page_aligned_bss;

in that case why not add __cacheline_aligned_bss?

Yinghai

Re: [PATCH v5] x86: make sure IDT is page aligned

[Kees Cook]

On Tue, Jul 16, 2013 at 1:21 PM, Yinghai Lu <yinghai@kernel.org> wrote:
> On Tue, Jul 16, 2013 at 11:34 AM, Kees Cook <keescook@chromium.org> wrote:
>> Since the IDT is referenced from a fixmap, make sure it is page aligned.
>> Merge with 32-bit one, since it was already aligned to deal with F00F
>> bug. Since bss is cleared before IDT setup, it can live there. This also
>> moves the other *_idt_table variables into common locations.
>>
>> This avoids the risk of the IDT ever being moved in the bss and having
>> the mapping be offset, resulting in calling incorrect handlers. In the
>> current upstream kernel this is not a manifested bug, but heavily patched
>> kernels (such as those using the PaX patch series) did encounter this bug.
>>
>> Signed-off-by: Kees Cook <keescook@chromium.org>
>> Reported-by: PaX Team <pageexec@gmail.com>
>> ---
>> v5:
>>  - add comments to all IDTs about alignment reasoning, suggested by Linus
>
> Where is thread for that?

That was off list, part of trying to coordinate this cleanup vs
minimal changes for the stable tree.

>> v4:
>>  - rework using __page_aligned_bss, suggested by Yinghai LU
>>  - move all the other IDT variables as well, suggested by HPA
>> v3:
>>  - merge 32-bit and 64-bit idt_table definition
>> v2:
>>  - 32-bit was already aligned
>> ---
>>  arch/x86/kernel/head_64.S    |   15 ---------------
>>  arch/x86/kernel/tracepoint.c |    6 ++----
>>  arch/x86/kernel/traps.c      |   12 ++++++------
>>  3 files changed, 8 insertions(+), 25 deletions(-)
>>
>> diff --git a/arch/x86/kernel/head_64.S b/arch/x86/kernel/head_64.S
>> index 5e4d8a8..e1aabdb 100644
>> --- a/arch/x86/kernel/head_64.S
>> +++ b/arch/x86/kernel/head_64.S
>> @@ -512,21 +512,6 @@ ENTRY(phys_base)
>>
>>  #include "../../x86/xen/xen-head.S"
>>
>> -       .section .bss, "aw", @nobits
>> -       .align L1_CACHE_BYTES
>> -ENTRY(idt_table)
>> -       .skip IDT_ENTRIES * 16
>> -
>> -       .align L1_CACHE_BYTES
>> -ENTRY(debug_idt_table)
>> -       .skip IDT_ENTRIES * 16
>> -
>> -#ifdef CONFIG_TRACING
>> -       .align L1_CACHE_BYTES
>> -ENTRY(trace_idt_table)
>> -       .skip IDT_ENTRIES * 16
>> -#endif
>> -
>>         __PAGE_ALIGNED_BSS
>>  NEXT_PAGE(empty_zero_page)
>>         .skip PAGE_SIZE
>> diff --git a/arch/x86/kernel/tracepoint.c b/arch/x86/kernel/tracepoint.c
>> index 4e584a8..1c113db 100644
>> --- a/arch/x86/kernel/tracepoint.c
>> +++ b/arch/x86/kernel/tracepoint.c
>> @@ -12,10 +12,8 @@ atomic_t trace_idt_ctr = ATOMIC_INIT(0);
>>  struct desc_ptr trace_idt_descr = { NR_VECTORS * 16 - 1,
>>                                 (unsigned long) trace_idt_table };
>>
>> -#ifndef CONFIG_X86_64
>> -gate_desc trace_idt_table[NR_VECTORS] __page_aligned_data
>> -                                       = { { { { 0, 0 } } }, };
>> -#endif
>> +/* No need to be aligned, but done to keep all IDTs defined the same way. */
>> +gate_desc trace_idt_table[NR_VECTORS] __page_aligned_bss;
>
> in that case why not add __cacheline_aligned_bss?

It seemed more correct to me to define all the IDTs the same, but
there was no technical reason for that, just one of regularity. I only
care about keeping the real IDT page aligned. :) I'm fine to do
whatever is deemed "correct". :)

-Kees

--
Kees Cook
Chrome OS Security

Re: [PATCH v5] x86: make sure IDT is page aligned

[Steven Rostedt]

On Tue, 2013-07-16 at 13:28 -0700, Kees Cook wrote:
> On Tue, Jul 16, 2013 at 1:21 PM, Yinghai Lu <yinghai@kernel.org> wrote:
> > On Tue, Jul 16, 2013 at 11:34 AM, Kees Cook <keescook@chromium.org> wrote:
> >> Since the IDT is referenced from a fixmap, make sure it is page aligned.
> >> Merge with 32-bit one, since it was already aligned to deal with F00F
> >> bug. Since bss is cleared before IDT setup, it can live there. This also
> >> moves the other *_idt_table variables into common locations.
> >>

> It seemed more correct to me to define all the IDTs the same, but
> there was no technical reason for that, just one of regularity. I only
> care about keeping the real IDT page aligned. :) I'm fine to do
> whatever is deemed "correct". :)

I'm actually unfamiliar with the F00F bug (heard of it, but have no idea
what it is). What happens if the F00F bug exists and we switch to an IDT
that's not paged aligned? Is that an issue?

-- Steve

Re: [PATCH v5] x86: make sure IDT is page aligned

[H. Peter Anvin]

The F00F big just means the IDT must be readonly.

Steven Rostedt <rostedt@goodmis.org> wrote:

>On Tue, 2013-07-16 at 13:28 -0700, Kees Cook wrote:
>> On Tue, Jul 16, 2013 at 1:21 PM, Yinghai Lu <yinghai@kernel.org>
>wrote:
>> > On Tue, Jul 16, 2013 at 11:34 AM, Kees Cook <keescook@chromium.org>
>wrote:
>> >> Since the IDT is referenced from a fixmap, make sure it is page
>aligned.
>> >> Merge with 32-bit one, since it was already aligned to deal with
>F00F
>> >> bug. Since bss is cleared before IDT setup, it can live there.
>This also
>> >> moves the other *_idt_table variables into common locations.
>> >>
>
>> It seemed more correct to me to define all the IDTs the same, but
>> there was no technical reason for that, just one of regularity. I
>only
>> care about keeping the real IDT page aligned. :) I'm fine to do
>> whatever is deemed "correct". :)
>
>I'm actually unfamiliar with the F00F bug (heard of it, but have no
>idea
>what it is). What happens if the F00F bug exists and we switch to an
>IDT
>that's not paged aligned? Is that an issue?
>
>-- Steve

-- 
Sent from my mobile phone. Please excuse brevity and lack of formatting.

Re: [PATCH v5] x86: make sure IDT is page aligned

[Kees Cook]

On Tue, Jul 16, 2013 at 1:33 PM, Steven Rostedt <rostedt@goodmis.org> wrote:
> On Tue, 2013-07-16 at 13:28 -0700, Kees Cook wrote:
>> On Tue, Jul 16, 2013 at 1:21 PM, Yinghai Lu <yinghai@kernel.org> wrote:
>> > On Tue, Jul 16, 2013 at 11:34 AM, Kees Cook <keescook@chromium.org> wrote:
>> >> Since the IDT is referenced from a fixmap, make sure it is page aligned.
>> >> Merge with 32-bit one, since it was already aligned to deal with F00F
>> >> bug. Since bss is cleared before IDT setup, it can live there. This also
>> >> moves the other *_idt_table variables into common locations.
>> >>
>
>> It seemed more correct to me to define all the IDTs the same, but
>> there was no technical reason for that, just one of regularity. I only
>> care about keeping the real IDT page aligned. :) I'm fine to do
>> whatever is deemed "correct". :)
>
> I'm actually unfamiliar with the F00F bug (heard of it, but have no idea
> what it is). What happens if the F00F bug exists and we switch to an IDT
> that's not paged aligned? Is that an issue?

Regardless of F00F, the IDT is now unconditionally being set up in a
fixmap entry (so that the unprivileged "sidt" instruction won't leak a
"real" kernel address, and so that this exposed address is read-only).
If the real IDT is not page aligned, the fixmap IDT will appear offset
and everything starts calling the wrong handlers.

The other IDTs don't need to be page aligned, but I marked them that
way in the clean up because it seemed sensible to define these tables
similarly. I can change the others to be __cacheline_aligned_bss if
that's desired.

-Kees

--
Kees Cook
Chrome OS Security

Re: [PATCH v5] x86: make sure IDT is page aligned

[H. Peter Anvin]

On 07/16/2013 01:47 PM, Kees Cook wrote:
> On Tue, Jul 16, 2013 at 1:33 PM, Steven Rostedt <rostedt@goodmis.org> wrote:
>> On Tue, 2013-07-16 at 13:28 -0700, Kees Cook wrote:
>>> On Tue, Jul 16, 2013 at 1:21 PM, Yinghai Lu <yinghai@kernel.org> wrote:
>>>> On Tue, Jul 16, 2013 at 11:34 AM, Kees Cook <keescook@chromium.org> wrote:
>>>>> Since the IDT is referenced from a fixmap, make sure it is page aligned.
>>>>> Merge with 32-bit one, since it was already aligned to deal with F00F
>>>>> bug. Since bss is cleared before IDT setup, it can live there. This also
>>>>> moves the other *_idt_table variables into common locations.
>>>>>
>>
>>> It seemed more correct to me to define all the IDTs the same, but
>>> there was no technical reason for that, just one of regularity. I only
>>> care about keeping the real IDT page aligned. :) I'm fine to do
>>> whatever is deemed "correct". :)
>>
>> I'm actually unfamiliar with the F00F bug (heard of it, but have no idea
>> what it is). What happens if the F00F bug exists and we switch to an IDT
>> that's not paged aligned? Is that an issue?
> 
> Regardless of F00F, the IDT is now unconditionally being set up in a
> fixmap entry (so that the unprivileged "sidt" instruction won't leak a
> "real" kernel address, and so that this exposed address is read-only).
> If the real IDT is not page aligned, the fixmap IDT will appear offset
> and everything starts calling the wrong handlers.
> 
> The other IDTs don't need to be page aligned, but I marked them that
> way in the clean up because it seemed sensible to define these tables
> similarly. I can change the others to be __cacheline_aligned_bss if
> that's desired.
> 

I'm fine keeping them as page aligned.  They are page-sized on x86-64
anyway (half page on i386).

	-hpa

Re: [PATCH v5] x86: make sure IDT is page aligned

[Yinghai Lu]

On Tue, Jul 16, 2013 at 3:03 PM, H. Peter Anvin <hpa@zytor.com> wrote:
> On 07/16/2013 01:47 PM, Kees Cook wrote:

>> The other IDTs don't need to be page aligned, but I marked them that
>> way in the clean up because it seemed sensible to define these tables
>> similarly. I can change the others to be __cacheline_aligned_bss if
>> that's desired.
>>
>
> I'm fine keeping them as page aligned.  They are page-sized on x86-64
> anyway (half page on i386).

ok, then should change

> +/* No need to be aligned, but done to keep all IDTs defined the same way. */
> +gate_desc trace_idt_table[NR_VECTORS] __page_aligned_bss;

==>

> +/* Only need to be cacheline aligned, but keep all IDTs defined the same way to be page aligned. */
> +gate_desc trace_idt_table[NR_VECTORS] __page_aligned_bss;

Re: [PATCH v5] x86: make sure IDT is page aligned

[H. Peter Anvin]

On 07/16/2013 03:13 PM, Yinghai Lu wrote:
> 
> ok, then should change
> 
>> +/* No need to be aligned, but done to keep all IDTs defined the same way. */
>> +gate_desc trace_idt_table[NR_VECTORS] __page_aligned_bss;
> 
> ==>
> 
>> +/* Only need to be cacheline aligned, but keep all IDTs defined the same way to be page aligned. */
>> +gate_desc trace_idt_table[NR_VECTORS] __page_aligned_bss;

It doesn't need to be cacheline aligned, either, to the best of my
knowledge.  The former comment is more correct.

	-hpa

[tip:x86/urgent] x86: Make sure IDT is page aligned

[tip-bot for Kees Cook]

Commit-ID:  4df05f361937ee86e5a8c9ead8aeb6a19ea9b7d7
Gitweb:     http://git.kernel.org/tip/4df05f361937ee86e5a8c9ead8aeb6a19ea9b7d7
Author:     Kees Cook <keescook@chromium.org>
AuthorDate: Tue, 16 Jul 2013 11:34:41 -0700
Committer:  H. Peter Anvin <hpa@linux.intel.com>
CommitDate: Tue, 16 Jul 2013 15:14:48 -0700

x86: Make sure IDT is page aligned

Since the IDT is referenced from a fixmap, make sure it is page aligned.
Merge with 32-bit one, since it was already aligned to deal with F00F
bug. Since bss is cleared before IDT setup, it can live there. This also
moves the other *_idt_table variables into common locations.

This avoids the risk of the IDT ever being moved in the bss and having
the mapping be offset, resulting in calling incorrect handlers. In the
current upstream kernel this is not a manifested bug, but heavily patched
kernels (such as those using the PaX patch series) did encounter this bug.

The tables other than idt_table technically do not need to be page
aligned, at least not at the current time, but using a common
declaration avoids mistakes.  On 64 bits the table is exactly one page
long, anyway.

Signed-off-by: Kees Cook <keescook@chromium.org>
Link: http://lkml.kernel.org/r/20130716183441.GA14232@www.outflux.net
Reported-by: PaX Team <pageexec@gmail.com>
Signed-off-by: H. Peter Anvin <hpa@linux.intel.com>
---
 arch/x86/kernel/head_64.S    | 15 ---------------
 arch/x86/kernel/tracepoint.c |  6 ++----
 arch/x86/kernel/traps.c      | 12 ++++++------
 3 files changed, 8 insertions(+), 25 deletions(-)

diff --git a/arch/x86/kernel/head_64.S b/arch/x86/kernel/head_64.S
index 5e4d8a8..e1aabdb 100644
--- a/arch/x86/kernel/head_64.S
+++ b/arch/x86/kernel/head_64.S
@@ -512,21 +512,6 @@ ENTRY(phys_base)
 
 #include "../../x86/xen/xen-head.S"
 	
-	.section .bss, "aw", @nobits
-	.align L1_CACHE_BYTES
-ENTRY(idt_table)
-	.skip IDT_ENTRIES * 16
-
-	.align L1_CACHE_BYTES
-ENTRY(debug_idt_table)
-	.skip IDT_ENTRIES * 16
-
-#ifdef CONFIG_TRACING
-	.align L1_CACHE_BYTES
-ENTRY(trace_idt_table)
-	.skip IDT_ENTRIES * 16
-#endif
-
 	__PAGE_ALIGNED_BSS
 NEXT_PAGE(empty_zero_page)
 	.skip PAGE_SIZE
diff --git a/arch/x86/kernel/tracepoint.c b/arch/x86/kernel/tracepoint.c
index 4e584a8..1c113db 100644
--- a/arch/x86/kernel/tracepoint.c
+++ b/arch/x86/kernel/tracepoint.c
@@ -12,10 +12,8 @@ atomic_t trace_idt_ctr = ATOMIC_INIT(0);
 struct desc_ptr trace_idt_descr = { NR_VECTORS * 16 - 1,
 				(unsigned long) trace_idt_table };
 
-#ifndef CONFIG_X86_64
-gate_desc trace_idt_table[NR_VECTORS] __page_aligned_data
-					= { { { { 0, 0 } } }, };
-#endif
+/* No need to be aligned, but done to keep all IDTs defined the same way. */
+gate_desc trace_idt_table[NR_VECTORS] __page_aligned_bss;
 
 static int trace_irq_vector_refcount;
 static DEFINE_MUTEX(irq_vector_mutex);
diff --git a/arch/x86/kernel/traps.c b/arch/x86/kernel/traps.c
index b0865e8..1b23a1c 100644
--- a/arch/x86/kernel/traps.c
+++ b/arch/x86/kernel/traps.c
@@ -63,19 +63,19 @@
 #include <asm/x86_init.h>
 #include <asm/pgalloc.h>
 #include <asm/proto.h>
+
+/* No need to be aligned, but done to keep all IDTs defined the same way. */
+gate_desc debug_idt_table[NR_VECTORS] __page_aligned_bss;
 #else
 #include <asm/processor-flags.h>
 #include <asm/setup.h>
 
 asmlinkage int system_call(void);
-
-/*
- * The IDT has to be page-aligned to simplify the Pentium
- * F0 0F bug workaround.
- */
-gate_desc idt_table[NR_VECTORS] __page_aligned_data = { { { { 0, 0 } } }, };
 #endif
 
+/* Must be page-aligned because the real IDT is used in a fixmap. */
+gate_desc idt_table[NR_VECTORS] __page_aligned_bss;
+
 DECLARE_BITMAP(used_vectors, NR_VECTORS);
 EXPORT_SYMBOL_GPL(used_vectors);
 

Re: [PATCH v5] x86: make sure IDT is page aligned

[Yinghai Lu]

On Tue, Jul 16, 2013 at 3:16 PM, H. Peter Anvin <hpa@zytor.com> wrote:
> On 07/16/2013 03:13 PM, Yinghai Lu wrote:
>>
>> ok, then should change
>>
>>> +/* No need to be aligned, but done to keep all IDTs defined the same way. */
>>> +gate_desc trace_idt_table[NR_VECTORS] __page_aligned_bss;
>>
>> ==>
>>
>>> +/* Only need to be cacheline aligned, but keep all IDTs defined the same way to be page aligned. */
>>> +gate_desc trace_idt_table[NR_VECTORS] __page_aligned_bss;
>
> It doesn't need to be cacheline aligned, either, to the best of my
> knowledge.  The former comment is more correct.

ok. so the old code is just for optimization to keep it cacheline aligned?

Re: [PATCH v5] x86: make sure IDT is page aligned

[H. Peter Anvin]

On 07/16/2013 04:39 PM, Yinghai Lu wrote:
> 
> ok. so the old code is just for optimization to keep it cacheline aligned?
> 

To the best of my knowledge.  I guess I should look through the git log
to make sure it isn't some old erratum fix.

	-hpa

Re: [PATCH v5] x86: make sure IDT is page aligned

[Yinghai Lu]

On Tue, Jul 16, 2013 at 4:43 PM, H. Peter Anvin <hpa@zytor.com> wrote:
> On 07/16/2013 04:39 PM, Yinghai Lu wrote:
>>
>> ok. so the old code is just for optimization to keep it cacheline aligned?
>>
>
> To the best of my knowledge.  I guess I should look through the git log
> to make sure it isn't some old erratum fix.
>
looks like before
commit e57113bc1ff591005ec0b0fb4885d97c01de73d8
Author: Jan Beulich <jbeulich@novell.com>
Date:   Sat Mar 25 16:30:01 2006 +0100

    [PATCH] x86_64: miscellaneous cleanup

    - adjust limits of GDT/IDT pseudo-descriptors (some were off by one)
    - move empty_zero_page into .bss.page_aligned
    - move cpu_gdt_table into .data.page_aligned
    - move idt_table into .bss
    - align inital_code and init_rsp
    - eliminate pointless (re-)declaration of idt_table in traps.c

it is PAGE aligned as it is after gdt page, after the commit, it is
cacheline aligned.


@@ -378,9 +378,12 @@ gdt_end:
        /* zero the remaining page */
        .fill PAGE_SIZE / 8 - GDT_ENTRIES,8,0

-ENTRY(idt_table)
-       .rept   256
-       .quad   0
-       .quad   0
-       .endr
+       .section .bss, "aw", @nobits
+       .align L1_CACHE_BYTES
+ENTRY(idt_table)
+       .skip 256 * 16

+       .section .bss.page_aligned, "aw", @nobits
+       .align PAGE_SIZE
+ENTRY(empty_zero_page)
+       .skip PAGE_SIZE

Re: [tip:x86/urgent] x86: Make sure IDT is page aligned

[Yinghai Lu]

On Tue, Jul 16, 2013 at 3:33 PM, tip-bot for Kees Cook <tipbot@zytor.com> wrote:
> Commit-ID:  4df05f361937ee86e5a8c9ead8aeb6a19ea9b7d7
> Gitweb:     http://git.kernel.org/tip/4df05f361937ee86e5a8c9ead8aeb6a19ea9b7d7
> Author:     Kees Cook <keescook@chromium.org>
> AuthorDate: Tue, 16 Jul 2013 11:34:41 -0700
> Committer:  H. Peter Anvin <hpa@linux.intel.com>
> CommitDate: Tue, 16 Jul 2013 15:14:48 -0700
>
> x86: Make sure IDT is page aligned
>
> Since the IDT is referenced from a fixmap, make sure it is page aligned.
> Merge with 32-bit one, since it was already aligned to deal with F00F
> bug. Since bss is cleared before IDT setup, it can live there. This also
> moves the other *_idt_table variables into common locations.
>
> This avoids the risk of the IDT ever being moved in the bss and having
> the mapping be offset, resulting in calling incorrect handlers. In the
> current upstream kernel this is not a manifested bug, but heavily patched
> kernels (such as those using the PaX patch series) did encounter this bug.
>
> The tables other than idt_table technically do not need to be page
> aligned, at least not at the current time, but using a common
> declaration avoids mistakes.  On 64 bits the table is exactly one page
> long, anyway.
>
> Signed-off-by: Kees Cook <keescook@chromium.org>
> Link: http://lkml.kernel.org/r/20130716183441.GA14232@www.outflux.net
> Reported-by: PaX Team <pageexec@gmail.com>
> Signed-off-by: H. Peter Anvin <hpa@linux.intel.com>

Hi, Peter,

Any reason for why following changelog get dropped?

---
v5:
 - add comments to all IDTs about alignment reasoning, suggested by Linus
v4:
 - rework using __page_aligned_bss, suggested by Yinghai LU
 - move all the other IDT variables as well, suggested by HPA
v3:
 - merge 32-bit and 64-bit idt_table definition
v2:
 - 32-bit was already aligned
---

That at least would help us to check if you apply the right version.

Yinghai

Re: [tip:x86/urgent] x86: Make sure IDT is page aligned

[H. Peter Anvin]

On 07/17/2013 11:57 AM, Yinghai Lu wrote:
> 
> Hi, Peter,
> 
> Any reason for why following changelog get dropped?
> 
> ---
> v5:
>  - add comments to all IDTs about alignment reasoning, suggested by Linus
> v4:
>  - rework using __page_aligned_bss, suggested by Yinghai LU
>  - move all the other IDT variables as well, suggested by HPA
> v3:
>  - merge 32-bit and 64-bit idt_table definition
> v2:
>  - 32-bit was already aligned
> ---
> 
> That at least would help us to check if you apply the right version.
> 

Procedurally, the changelogs don't belong in commit messages.  I tend to
leave them in if they are particularly illustrative or if the commit
message only makes sense with the additional context, but neither of
those is really ideal.

Technically, because of the --- line which indicates the end of the
commit message.

	-hpa

Re: [tip:x86/urgent] x86: Make sure IDT is page aligned

[Ingo Molnar]

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

> On 07/17/2013 11:57 AM, Yinghai Lu wrote:
> > 
> > Hi, Peter,
> > 
> > Any reason for why following changelog get dropped?
> > 
> > ---
> > v5:
> >  - add comments to all IDTs about alignment reasoning, suggested by Linus
> > v4:
> >  - rework using __page_aligned_bss, suggested by Yinghai LU
> >  - move all the other IDT variables as well, suggested by HPA
> > v3:
> >  - merge 32-bit and 64-bit idt_table definition
> > v2:
> >  - 32-bit was already aligned
> > ---
> > 
> > That at least would help us to check if you apply the right version.
> > 
> 
> Procedurally, the changelogs don't belong in commit messages.  I tend to
> leave them in if they are particularly illustrative or if the commit
> message only makes sense with the additional context, but neither of
> those is really ideal.
> 
> Technically, because of the --- line which indicates the end of the
> commit message.

Furthermote, the -tip notification email will generally email-thread on 
lkml to the patch submission that was applied.

That is a more robust indication of which submission was applied than any 
changelog detail.

Thanks,

	Ingo